Remove global variable

[quink-black]

No description provided.

[maxsharabayko]

CI build failure. Not all places of use were updated.
Please build with -DENABLE_EXPERIMENTAL_BONDING=ON to see more places.

[quink-black]

CI build failure. Not all places of use were updated.
Please build with -DENABLE_EXPERIMENTAL_BONDING=ON to see more places.

Thanks for the review, I missed that part. A new version was pushed with double check by grep 's_UDTUnited'.

[quink-black]

Finally CI build success on all platforms.

[ethouris]

What exactly problem does this solve?

[quink-black]

What exactly problem does this solve?

Trying to avoid non-plain-old-data type global variables. The second commit is an example of what kind of problem does non-POD global variable have. And there are other kind of issues like exception handling.

[maxsharabayko]

@quink-black Could you please update the PR to the latest master (resolve conflicts)?

[maxsharabayko]

How would the CUDTUnited object be destructed?

[quink-black]

I don't know how to delete the singleton automatically in a portable way.

CUDT::cleanup is a good entry to trigger delete, then another global lock is needed to protect CUDT::startup() and CUDT::cleanup() called by multiple threads.

What's your suggestion?

[quink-black]

How about this:

static CUDTUnited *getInstance()
{
    static CUDTUnited instance;
    return &instance;
}

static void createInstance() {
    getInstance();
}

CUDTUnited* srt::CUDT::uglobal()
{
    pthread_once(&s_UDTUnitedOnce, createInstance);
    return getInstance();
}

[quink-black]

Or just do cast and remove the createInstance()

[maxsharabayko]

To reduce dereferencing, this would be a more convenient function.

Suggested change

	CUDTUnited* CUDT::uglobal()
	{
	static CUDTUnited instance;
	return &instance;
	}
	CUDTUnited& CUDT::uglobal()
	{
	static CUDTUnited instance;
	return instance;
	}

[quink-black]

I don't think there is performance difference between pointer and reference. They are compiled to the same machine code.

If the singleton needs to be deleted as the previous review comments, then delete a reference looks odd.

[maxsharabayko]

Hi @quink-black. Sorry for a notable pause in the review.
I think the PR looks good now.

I am only still thinking about srt::CUDT::uglobal() returning a reference instead of a pointer. I agree there would be no performance difference between pointer and reference. The main reason I believe it would be better is that with a reference we always know we don't ever return a NULL object, and we don't need to check it in various places in the code.

If you still believe a pointer is better, then it might be worth marking a function with __attribute__((returns_nonnull)) (placed in srt_attr_defs.h).

[quink-black]

Hi @quink-black. Sorry for a notable pause in the review.
I think the PR looks good now.

I am only still thinking about srt::CUDT::uglobal() returning a reference instead of a pointer. I agree there would be no performance difference between pointer and reference. The main reason I believe it would be better is that with a reference we always know we don't ever return a NULL object, and we don't need to check it in various places in the code.

Good point! I have added a new commit, with a lot of small changes from pointer to reference.