THRIFT-5678: lib: cpp: TConnectedClient: warning due to non-virtual dtor

[cfriedt]

Commit 042580f removed the virtual keyword from the declaration of ~TConnectedClient().

While mostly benign, it does cause a warning in some versions of GCC (non-virtual destructors can result in undefined behaviour), which can throw off CI sometimes when building with -Werror.

• Did you create an Apache Jira ticket? (not required for trivial changes)
• If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
• Did you squash your changes to a single commit? (not required, but preferred)
• Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
• If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

[cfriedt]

@Jens-G @emmenlau - let me know your preferences. E.g. ensure all destructors are virtual, inline in .h vs in .cpp, etc.

[Jens-G]

Not sure what the status is here?

[cfriedt]

I opted to only insert the virtual keyword for destructors in this PR for now.

I know that @emmenlau also wanted to remove inline implementations that assign default in headers. If that's still desireable, I can do a quick scan to ensure that all classes declared withdefault destructors in .h files have some sort of presence in .cpp files - it would be better to not have to add a .cpp file purely to declare a destructor as default.

[Jens-G]

Any idea how to continue with this? I would rather not decide about this one alone.

[CJCombrink]

My 2c: override implies virtual. It is both a duplication and not conventional pattern to have both specified for the same function (I think SonarQube will warn on this and say the virtual should be removed).
See override specifier

In a member function declaration or definition, override specifier ensures that the function is virtual and is overriding a virtual function from a base class.

If this update is made at least the PR will touch a bit fewer files since this change is bigger than it should be

Also refer to this core guideline: C.128: Virtual functions should specify exactly one of virtual, override, or final

[CJCombrink]

Looking at 042580f yes virtual was removed from lib/cpp/src/thrift/server/TConnectedClient.h but override was added.
This enforces that the destructors are virtual for at least that type.

it does cause a warning in some versions of GCC

I think one would need to check specifically which classes gives this warning, since I expect it would not be TConnectedClient (since it is correct, virtual destructor)

Looking through this PR, I see there are actually a few classes that does not have virtual destructors, like TConcurrentClientSyncInfo.h. The question would probably be if they are base classes anywhere. Thus a change like this can be valuable.

@cfriedt I would suggest revert any change that adds virtual to a destructor already marked as override and then see where that leaves the PR.

[CJCombrink]

I would suggest revert any change that adds virtual to a destructor already marked as override and then see where that leaves the PR.

[cfriedt]

Sure - thanks for the feedback.

There is one other change that I would need to do to upgrade the Thrift version used in Zephyr after this. That is, namely, to redo the change where I added an alternative to some boost function (it's been so long I'm having a hard time remembering which).

Additionally, we are one SDK release away from supporting std::thread and friends, which is exciting.

And after that, I was thinking it would be interesting to try and coalesce fbthrift here too, because the newer compiler apparently generates really efficient c++ code with coroutines.