Why must I declare copy & move constructors when declaring destructors?

10
votes

I have a NonCopyable class and an Application class derived from NonCopyable:

class NonCopyable
{
public:
    NonCopyable() = default;
    virtual ~NonCopyable() = default;
    NonCopyable(NonCopyable const&) = delete;
    NonCopyable& operator =(NonCopyable const&) = delete;
    NonCopyable(NonCopyable&&) = delete;
    NonCopyable& operator=(NonCopyable&&) = delete;
};

class Application : public NonCopyable
{
public:
    ~Application() { /* ...delete stuff... */ }
};

As you see, I don't need to redeclare deleted move constructors or assignment operators as I've declared them in the base class. But why does Resharper suggest me to declare other special member functions? It said:

Class Application defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [hicpp-special-member-functions].

There're also [cppcoreguidelines-special-member-functions] reminders with the same message.

The current workaround is to explicitly write all of them to make the warning go away:

class Application : public NonCopyable
{
public:
    ~Application() { /* ...delete stuff... */ }
    Application(Application const&) = delete;
    Application& operator =(Application const&) = delete;
    Application(Application&&) = delete;
    Application& operator=(Application&&) = delete;
};
3
c++c++11resharper
Please have a look at the rule of three / five / zero. – Ron
If you need to "delete stuff" in the destructor, you most often need to "copy stuff" in the copy constructor. – Bo Persson
@BoPersson For this example, I don't need another Application instance (singleton) so there isn't a need of copy constructor. – MiP
Hi MiP, Resharper has its own logic based on which it create a list of warning. If warning are not fatal you can just ignore it. It not necessary that you have done some mistake. – Abhijit Pritam Dutta
This particular inspection is coming from clang-tidy, not from ReSharper itself (it just invokes clang-tidy automatically on your source code). You can distinguish clang-tidy inspections by their name in square brackets at the end of the message. You can also quickly consult inspection documentation by pressing Alt+Enter on the inspection, then going into inspection submenu and selecting Open documentation. In the same submenu you can change inspection severity or switch it off altogether. – Igor Akhmetov

3 Answers

4
votes

More often than not, if a class defines a destructor that differs from what the compiler would automatically generate, its purpose is to release some resource that is managed by the class.

If there is some resource that a destructor must release then there is usually also a need for a user-defined constructor to initialise that resource, a user-defined copy constructor to create a copy of the resource owned by an existing object of the same type, and a user-defined move constructor to assume ownership of the resource from a temporary object that is ceasing to exist.

Similarly, there will also be a need for an copy assignment operator and move assignment operator, so that expressions like a = b will work to copy or move the resource to an existing objects.

If the class does not have one or more of these constructors or assignment operators then, practically, code which uses several objects often will not behave consistently (e.g. a destructor releasing a resource twice because it is shared by two objects, resulting in undefined behaviour, etc).

Of course, there are circumstances in which a class does not need the complete set of constructors, assignment operators, and destructor. However, leaving one of them out is often a programmer error (as distinct from an intended effect) so code analysis tools will often complain about such cases.

2
votes

default, delete are not inherited

You are wrongfully assuming some kind of transitivity (When constructing an object, I always need to use the superclass constructor with a similar signature).

Here is a counter-example :

#include <string>
#include <iostream>

class NonCopyable
{
public:
    NonCopyable() = default; // Added this little fella
    virtual ~NonCopyable() = default;
    NonCopyable(const NonCopyable &) = delete;
    NonCopyable& operator =(NonCopyable const&) = delete;
    NonCopyable(NonCopyable&&) = delete;
    NonCopyable& operator=(NonCopyable&&) = delete;
};

class Application : public NonCopyable
{
public:
    Application() = default;
    Application(const Application& a) : NonCopyable(), x(a.x){}
    ~Application() { /* ...delete stuff... */ }
    std::string x;
};

int main(){
   Application a;
   a.x = "Hello";
   Application b(a);

   std::cout << b.x << std::endl;

}

As you can see I am perfectly able to declare copy constructor of a subclass by using the only available constructor of the superclass. I could also declare the other constructors of the rule of 5. This is why static analyzers are giving warning about the rule of 5 .

Conclusion : NonCopyable is a useless class.

1
votes

For all ReSharper knows, you defined a non-trivial destructor and did not follow up with the rest of the rule of 3/5. Yes, the copy/move constructors are deleted by default since they are deleted in the base class, but how would ReSharper know this is desirable behavior? Therfore it makes sense for it to warn you unless you explicitly mark them as delete.

You can just disable the warning locally with a pragma or delete the constructors/assignments explicitly as you suggested.

As a workaround you can introduce a virtual cleanup() method, call it from the NonCopyable destructor, and override it (instead of the destructor) in the derived classes.