How to resolve the sonarqube static code analysis error "Explicitly define the missing copy constructor, move constructor, copy assignment operator and move assignment operator so that they will not be implicitly provided" in C++

johnpaul · May 19, 2020, 4:51pm

Hello all,

I am getting the below sonarqube static code analysis error:

Explicitly define the missing copy constructor, move constructor, copy assignment operator and move assignment operator so that they will not be implicitly provided.

I am getting the above message at the below destructor declaration in the header file:

~CCPSDataManager();

And also in my .cpp file, there is definition for the destructor ~CCPSDataManager().

Here do I need to follow the rule of 5 by providing the destructor, copy constructor and the copy-assignment operator, move constructor and the move-assignment operator? Or is there any other approach?

And also If I define the copy constructor, copy-assignment operator, move constructor and the move-assignment operator we are writing many lines of code without using those. Is this a correct approach?

Please suggest and give me some insight on how to proceed?

JolyLoic · May 19, 2020, 8:56pm

You are asking very good questions, for which we can’t give accurate answers without knowing more about your code. But we can provide a general strategy.

First, yes, providing all those functions is cumbersome. There are some cases where you really need to, but those cases are rather rare. So here is a proposed action plan.

Are you sure you are not using those? Those functions can be called implicitly by the compiler in many places, so if you’re sure you are not using them, or more precisely, that you don’t need / don’t want them, one way to satisfy this rule is to declare them as deleted. That way, the compiler no longer generates them, and if your code still compiles, well, great, you have solved the issue, if it no longer does, it means that you were using them, and since you use the default version, which is probably wrong in this case, you might have just uncovered an bug, congratulations!

If you go that way, no need to write the body for those functions, but you still need to declare them (technically, you can reduce the number of lines to write, by just having a deleted move assignment operator, because in that case all the other special member functions get deleted too, but it might not be obvious to everybody). Less cumbersome, but maybe we can do better.

class CCPSDataManager {
public:
  ~CCPSDataManager();
  CCPSDataManager(CCPSDataManager const &) = delete;
  CCPSDataManager(CCPSDataManager &&) = delete;
  CCPSDataManager& operator=(CCPSDataManager const &) = delete;
  CCPSDataManager& operator=(CCPSDataManager &&) = delete;
};

Usually, the best answer is not to aim for the rule of five, but for the rule of zero. By asking why do you need the destructor in the first place. There are usually two possible reasons :

Because CCPSDataManager is the base class of a polymorphic hierarchy. In that case, if this destructor is defaulted or empty, we are not raising a violation of the rule of five, so everything should be fine.
Because you are manually managing a resource, and you need to do some clean-up in the destructor. If your class only goal is to manage this resource, go ahead with the full rule of 5. But in many cases, you can factor out this management in a utility class (that you write only once, or maybe even less, classes like unique_ptr are already very good at handling many manual ressources).

If you struggle with that, maybe we could help you, if you provide a little bit more context about why you need the destructor.

So, as a summary:

If you don’t need the destructor, just remove it
If you need it, try to move the management of this resource in a dedicated class (that conforms to the rule of 5)
If you are writing a class for resource management, see if you can make it non-copiable and not-moveable, and declare the matching operations as =delete
If you can’t, try to make it non-copiable but movable, and mark copying operations as =delete
If you can’t, go for the full rule of five, declare & define those five functions.

Hope this helps!

johnpaul · May 20, 2020, 7:44am

Thank you @JolyLoic for the detailed explanation:

My understanding based on your inputs:

sonarqube is giving the error because the compiler is implicitly looking for the definition of at least one of the remaining four functions.

So if i am not sure which function the compiler is implicitly trying to invoke then,

can i make all the remaining four operations as = delete

And once I compiled, based on the error/warning can i define the function which is required?

For the two possible reasons you mentioned:

Scenario 1:

In my scenario, no class is inheriting from the CCPSDataManager class. We are using CCPSDataManager in another class with shared_ptr like as shown below:

shared_ptr<CCPSDataManager> l_hDataManager = CCPSDataManager::getInstance();

And also regarding managing a resource, we are using shared_ptr to handle the resource and that resource we are using in the destructor.

I think we can delete the line m_ActiveInstance = NULL; from the destructor as the resource is of type shared_ptr.

And also we can remove the destructor definition, declaration and can follow the rule of zero.

Below is the code snippet:

.h file

static shared_ptr<CSDKDataManager>    m_ActiveInstance;

.cpp file

CCPSDataManager :: ~CCPSDataManager()
{
    m_ActiveInstance = NULL;
}

shared_ptr<CCPSDataManager> CCPSDataManager ::getInstance(int f_nStartedWithUI)
{    
    if (m_ActiveInstance == NULL)
    {
        m_ActiveInstance = shared_ptr<CCPSDataManager> (new (std::nothrow) CCPSDataManager(f_nStartedWithUI));
    }
    return m_ActiveInstance;
}

Scenario 2:

In the below scenario, we are using the destructor to write the logs and also to do the third party library cleanup.
So in this case i assume we should go with rule of 5 by retaining the destructor andd add copy constructor, the copy-assignment operator, move constructor and the move-assignment operator.

Below is the code snippet:

.h file

  ~CInstructionTask();

.cpp file

CInstructionTask::~CInstructionTask()
{
    writeLog(LOG_INFORMATION, __FUNCTION__, __LINE__, " cleaning of destructor.");
    /* Shutdown client library. */
    client_cleanup();
}

Please suggest your approach for the above two scenarios.

JolyLoic · May 20, 2020, 8:24pm

Not exactly. We give the error because even if the compiler does not need one of the missing definition now, it might need if the code is updated. And since most users do not analyze the code each time they build, we prefer to promote robust code and good practices, even if the code does not yet contain an actual bug.

Scenario 1

I don’t quite understand how the system is supposed to work. The only way for CCPSDataManager to be destroyed is when the reference count goes to 0, which means that m_ActiveInstance no longer holds an instance of CCPSDataManager. Only then the destructor will be called, but there is no need to reset m_ActiveInstance in this situation, because it is already empty.

So, yes, apparently, just removing the destructor and following the rule of 0 is the way to go.

Scenario 2

Logging

I don’t know your logging needs, so I’m not sure if what I propose is going to be what you need, but here are some thoughts:

Do you really need those logs? It looks to me more like a debugging tool than like a user feature… (solution 1 of my previous list)
If you need these low-level logs, I think you probably also need to log when an object is created, including through copy-construction and move-construction, so this means the rule of 5 would be of help
If you need to log at that level, you usually need to do it for more than one class. So it would be nice to be able to factor this out.

One solution would then be to create a dedicated class for log, and use this class in CInstructionTask. Something like:

template<class T> // The template can help if you want to count the living instances of different classes
class InstanceLogger {
  ~InstanceLogger() {
    writeLog(LOG_INFORMATION, __FUNCTION__, __LINE__, " cleaning of destructor.");
  }
  InstanceLogger(InstanceLogger const &) {
    writeLog(LOG_INFORMATION, __FUNCTION__, __LINE__, " creation by copy.");
  }
  // Continue until you have the 5 special member functions
};

Then in you class, you just add a member:

class CInstructionTask {
  InstanceLogger<CInstructionTask> logger;
  // Destruction and other operations will be logged, without having 
  // to define a destructor in this class
};

This is solution 2 of my previous list

Cleaning the client library

I’m not sure what this exactly means. I assume that this library must be set-up only once, and cleaned once and only once too. In that case, you should make sure that CInstructionTask cannot be copied, otherwise, the library may be used while already cleaned, and will end-up being cleaned several times.

You can to that by deleting the copy constructor, move constructor… (solution 3 of the list)

And maybe, depending on the nature of this client library, it can be possible to wrap it in a dedicated class, which would mean that CInstructionTask can once more be destructor-free (back to solution 2).

Creating these dedicated classes for resource management might seem to a lot of work just to avoid defining some special member functions, but it usually pays off:

They can usually be re-used across the code
Since they are focussed on one subject only, they are usually more simple to write, and to test
Writing a class that manually manages several resources is quite hard, especially for error recovery, but with this strategy, this can no longer happen.

johnpaul · May 21, 2020, 3:54am

Thank you @JolyLoic for your valuable inputs.

system · May 28, 2020, 3:55am

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.