isocpp / CppCoreGuidelines

The C++ Core Guidelines are a set of tried-and-true guidelines, rules, and best practices about coding in C++
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines
Other
41.96k stars 5.4k forks source link

Never delete a move constructor or move assignment operator #1670

Open JordanMaples opened 3 years ago

JordanMaples commented 3 years ago

As quoted from @jwakely in https://github.com/microsoft/GSL/pull/842#issuecomment-673403244

Never delete a move constructor or move assignment operator. If you want moving to be equivalent to copying then do not write move ctor/assignment, and ensure there is a user-declared copy ctor or copy assignment operator or both (or a user-declared destructor) N.B. user-declared not user-provided, which means not_null(const not_null&) = default; is fine.

cubbimew commented 3 years ago

A counterexample is ClonableBase from C.21: If you define or =delete any copy, move, or destructor function, define or =delete them all but I think there could be a useful guideline here.

jwakely commented 3 years ago

Yes, an absolute statement like "never" is never correct :-)

Maybe "never delete it unless you want to prevent moving and copying".

hsutter commented 3 years ago

Editors call: @cubbimew please create a new guideline along these lines. Thanks!

KindDragon commented 1 year ago

So, it is important when we have a template member of a class and depending on who it is a move constructor or move assignment operator will be generated from that. Are there other cases where we shouldn't declare them?

Quuxplusone commented 1 month ago

A counterexample is ClonableBase from C.21: If you define or =delete any copy, move, or destructor function, define or =delete them all ...

In my book, CloneableBase is no exception, and C.21 is currently over-strict. The best way to make a type like CloneableBase immovable is not to write out five different defaulted-or-deleted signatures.

Right now, C.21 recommends:

class CloneableBase {
public:
    virtual unique_ptr<CloneableBase> clone() const;
    virtual ~CloneableBase() = default;
    CloneableBase() = default;
    CloneableBase(const CloneableBase&) = delete;
    CloneableBase& operator=(const CloneableBase&) = delete;
    CloneableBase(CloneableBase&&) = delete;
    CloneableBase& operator=(CloneableBase&&) = delete;
    // ... other constructors and functions ...
};

But it is strictly better to recommend:

class CloneableBase {
public:
    explicit CloneableBase() = default;
    CloneableBase(const CloneableBase&) = delete;
    CloneableBase& operator=(const CloneableBase&) = delete;  // alternatively, `void operator=`
    virtual ~CloneableBase() = default;

    virtual unique_ptr<CloneableBase> clone() const;
    // ... other constructors and functions ...
};

That is, "Follow the Rule of Five for types that have transferrable-resource-ownership semantics. Follow the Rule of Three for types that are meant to be immovable." And in fact, for a non-polymorphic immovable type, like lock_guard, it is reasonable to write:

class LockGuard {
public:
    explicit LockGuard(std::mutex&);
    LockGuard(const LockGuard&) = delete;
    void operator=(const LockGuard&) = delete;

    // ...other functions...
};

This is basically the "Rule of Zero," but with a sidecar that if you're trying to delete the copy operations, you should delete the two of them (but still follow the "Rule of Zero" for the destructor, since you're not doing anything to it).