What to do with cpp:S4963 when destructor is mandatory

Clean Code Report False-positive / False-negative...
1

Context

Observed behavior

Rule cpp:S4963 is complaining about having a defined destructor in my class, but this desctructor is needed in order to join several threads in a specific order so it can neither be deleted nor defaulted.

Steps to reproduce

Sample code being reported below:

class MyClass {
public:  
    MyClass () = default;
    ~MyClass() {
        // Some mandatory code
    }
};

As the issue description mentions several exceptions:

Exceptions

  • Empty destructors are treated as though they were defaulted.
  • There are several cases when this rule should not be followed. For instance, if your class is manually handling a resource, logging when being constructed/copied, maintaining some kind of counter, having non-transient data that should not be copied (like capacity for std::vector)…​ In that case, it should still follow the rule of 5 (S3624). And you should consider if you can isolate this specific behavior in a base class or a dedicated member data, which would allow you to still follow the rule of 0.

I tried to follow the rule of 5 instead and defined all unused operators/constructors as deleted:

class MyClass {
public:  
    MyClass () = default;
    MyClass & operator=(const MyClass& other) = delete;
    MyClass (MyClass &&other) noexcept = delete;
    MyClass const & operator=(MyClass &&other) = delete;
    ~MyClass() {
        // Some mandatory code
    }
};

But rule cpp:S4963 keeps triggering and keeps asking to remove the destructor…

Question

Now what? How am I supposed to solve this issue in a clean way?

EDIT: I just read How to fix a 'The "Rule-of-Zero" should be followed'? - #12 by Tohnmeister which seems to discuss a similar topic, but I did not understand why rule of Zero still applies when you used rule of Five.

Are you supposed to manually change the Sonarqube settings to disable cpp:S4963 en only keep cpp:S3624 or something? If so, having to change the Sonarqube conf does not seem like a viable approach to me as this is ofthen not possible when contributing to public repositories where you have no rights to do that nor to close issues as “won’t fix”…

As a side note, cpp:S3624 never triggered for me, only cpp:S4963 did.

5

Hello @FlorentP42,

Thank you for raising your question and sharing your experience.

I think it did well. If you were not doing any logging in your destructor, I would have advised using std::jthread instead of std::thread. Quoting the documentation, std::jthread has the same general behavior as std::thread, except that std::jthread automatically rejoins on destruction and can be canceled/stopped in certain situations. This would essentially allow you to follow the rule of zero here.

I appreciate this may be surprising, and the topic is not easy. I raised a ticket to improve how we explain and raise issues on code breaking these rules.

I’ll try to rephrase and expand on what Abbas suggested in the other thread.

  1. Essentially, the rule of zero should be followed whenever possible. If it can’t, the rule of five should be followed.
  2. Yet, we still raise an issue recommending the rule of zero in some situations like the one you described. This reminds users that there can still be better alternatives, for example, by re-using existing RAII-compatible types as member variables (such as std::jthread for a class only slightly different than yours).
  3. When reviewing the issue, you can close it as “won’t fix” if the alternatives are unsuitable in your context.

We also have these two blog posts that dive deeper into this subject:

You should turn off a rule only if you intentionally do not want to follow it, say because it is never (or rarely) relevant to your project.

I hope this helps.

6

I think this would not have helped either… I need to close specific threads in a specific order: Even having RAII thread objects in my class, I have no way to guarantee in what order they will be freed by a default destructor, do I?
Also our project relies on C++11 and this is a C++20 feature, so this would not have been an option anyway…

As I said above, sadly I cannot do that on Sonarcloud of public repositories, I do not have the ability to close a reported code smell as “won’t fix”, so there is no valid solution in my case with what you have suggested so far…

7

Hello @FlorentP42

Destructors are run in a deterministic manner in C++ but the order of element destruction in STL containers is not specified by the standard.

However, delete [] does delete objects in reverse order they were created. So depending on your implementation, the order for std::vector is fixed and appropriate for your use case. I would not advise relying on this implementation detail and instead creating a custom container class that handles destruction in the order you need. This helps keep classes smaller and abides by the single-responsibility principle (SRP), thus reducing maintenance burden and complexity.

In the same fashion, since you cannot use std::jthread, having a wrapper around std::thread that joins in its destructor helps follow the SRP. We link to a C++ Core Guildeline in C++ static code analysis: "std::jthread" should be used instead of "std::thread" that details this a bit more.

I missed this bit indeed. I would advise discussing with the owner of the repository to define a process to close these issues. If they want to give you permission to handle issues yourself, they can. See for example this thread which links to Managing user permissions in SonarCloud.

HTH