Should I Delete the Move Constructor and the Move Assignment of a Smart Pointer

Should I delete the move constructor and the move assignment of a smart pointer?

Guideline

Never delete the special move members.

In typical code (such as in your question), there are two motivations to delete the move members. One of those motivations produces incorrect code (as in your example), and for the other motivation the deletion of the move members is redundant (does no harm nor good).

1. If you have a copyable class and you don't want move members, simply don't declare them (which includes not deleting them). Deleted members are still declared. Deleted members participate in overload resolution. Members not present don't. When you create a class with a valid copy constructor and a deleted move member, you can't return it by value from a function because overload resolution will bind to the deleted move member.

2. Sometimes people want to say: this class is neither movable nor copyable. It is correct to delete both the copy and the move members. However just deleting the copy members is sufficient (as long as the move members are not declared). Declared (even deleted) copy members inhibit the compiler from declaring move members. So in this case the deleted move members are simply redundant.

If you declare deleted move members, even if you happen to pick the case where it is redundant and not incorrect, every time someone reads your code, they need to re-discover if your case is redundant or incorrect. Make it easier on readers of your code and never delete the move members.

The incorrect case:

struct CopyableButNotMovble
{
    // ...
    CopyableButNotMovble(const CopyableButNotMovble&);
    CopyableButNotMovble& operator=(const CopyableButNotMovble&);
    CopyableButNotMovble(CopyableButNotMovble&&) = delete;
    CopyableButNotMovble& operator=(CopyableButNotMovble&&) = delete;
    // ...
};

Here is example code you probably expected to work with CopyableButNotMovble but will fail at compile time:

#include <algorithm>
#include <vector>

struct CopyableButNotMovble
{
    // ...
    CopyableButNotMovble(const CopyableButNotMovble&);
    CopyableButNotMovble& operator=(const CopyableButNotMovble&);
    CopyableButNotMovble(CopyableButNotMovble&&) = delete;
    CopyableButNotMovble& operator=(CopyableButNotMovble&&) = delete;

    CopyableButNotMovble(int);
    // ...
    friend bool operator<(CopyableButNotMovble const& x, CopyableButNotMovble const& y); 
};

int
main()
{
    std::vector<CopyableButNotMovble> v{3, 2, 1};
    std::sort(v.begin(), v.end());
}

In file included from test.cpp:1:
algorithm:3932:17: error: no
      matching function for call to 'swap'
                swap(*__first, *__last);
                ^~~~
algorithm:4117:5: note: in
      instantiation of function template specialization 'std::__1::__sort<std::__1::__less<CopyableButNotMovble,
      CopyableButNotMovble> &, CopyableButNotMovble *>' requested here
    __sort<_Comp_ref>(__first, __last, __comp);
    ^
algorithm:4126:12: note: in
      instantiation of function template specialization 'std::__1::sort<CopyableButNotMovble *,
      std::__1::__less<CopyableButNotMovble, CopyableButNotMovble> >' requested here
    _VSTD::sort(__first, __last, __less<typename iterator_traits<_RandomAccessIterator>::value_type>());
           ^
...

(many nasty error messages from deep inside your std::lib)

The correct way to do this is:

struct CopyableButNotMovble
{
    // ...
    CopyableButNotMovble(const CopyableButNotMovble&);
    CopyableButNotMovble& operator=(const CopyableButNotMovble&);
    // ...
};

The redundant case:

struct NeitherCopyableNorMovble
{
    // ...
    NeitherCopyableNorMovble(const NeitherCopyableNorMovble&) = delete;
    NeitherCopyableNorMovble& operator=(const NeitherCopyableNorMovble&) = delete;
    NeitherCopyableNorMovble(NeitherCopyableNorMovble&&) = delete;
    NeitherCopyableNorMovble& operator=(NeitherCopyableNorMovble&&) = delete;
    // ...
};

The more readable way to do this is:

struct NeitherCopyableNorMovble
{
    // ...
    NeitherCopyableNorMovble(const NeitherCopyableNorMovble&) = delete;
    NeitherCopyableNorMovble& operator=(const NeitherCopyableNorMovble&) = delete;
    // ...
};

It helps if you make a practice of always grouping all 6 of your special members near the top of your class declaration, in the same order, skipping those you don't want to declare. This practice makes it easier for readers of your code to quickly determine that you have intentionally not declared any particular special member.

For example, here is the pattern I follow:

class X
{
    // data members:

public:
    // special members
    ~X();
    X();
    X(const X&);
    X& operator=(const X&);
    X(X&&);
    X& operator=(X&&);

    // Constructors
    // ...
};

Here is a more in-depth explanation of this declaration style.

Why should I delete move constructor and move assignment operator in a singleton?

If you declare a copy constructor (even if you define it as deleted in the declaration), no move constructor will be declared implicitly. Cf. C++11 12.8/9:

If the definition of a class X does not explicitly declare a move constructor, one will be implicitly declared as defaulted if and only if

— X does not have a user-declared copy constructor,

— ...

Since you do have a user-declared copy constructor, there won't be a move constructor at all if you don't declare one. So you can just get rid of the move constructor declaration-definition entirely. Same for the move-assignment operator.

Should I mark the move constructor & move assignment operator as deleted in this case?

I think your question answers itself.

I don't hope the user to use the instance of Foo class directly, I hope they always use std::shared_ptr<Foo> instead.

followed by

Foo foo1(std::move(foo));

demonstrating the ability to use an instance of Foo directly, not managed by a shared pointer. Defining the move constructor as deleted prevents this thing you want users to avoid.

Move Constructor and = operator for Shared Pointer

Your copy and move constructors are equivalent to the implicit ones. Remove them. You don't need to write them out explicitly as the copy and move constructor of the std::shared_ptr correctly implement both operations.

Or do I need to to call rhs.transform.reset() to de-allocate the rhs after the move?

No, the moved-from object will lose ownership after the move:

shared_ptr(shared_ptr&& r) noexcept;
template<class Y> shared_ptr(shared_ptr<Y>&& r) noexcept;

Remark: The second constructor shall not participate in overload resolution unless Y* is convertible to T*.

Effects: Move-constructs a shared_ptr instance from r.
Postconditions: *this shall contain the old value of r. r shall be empty. r.get() == nullptr.

As for the copy and move-assignment operators, they too will be equivalent. Move-assignment will correctly transfer ownership and the copy-constructor will perform a shallow copy so that both shared_ptrs have ownership.

If a shallow copy (shared ownership) is really want you want, then shared_ptr is the correct tool. Otherwise I'd suggest using unique_ptr if you want to implement unique ownership.

Should i delete a moved from dynamic pointer

std::move does not move anything. It merely casts an l-value reference to an r-value reference.

In the example you give, the pointer p has not moved anywhere.

BTW. please don't deal in raw pointers - use a std::unique_ptr instead.

This allocates memory for an int, initialises the int to the value of 42 and stores the address of that memory in p.

int *p(new int(42));

---

This casts the int pointed to by p to an int&& and then constructs the int val from that r-value reference. Since int is an integral type, a construction from an r-value (i.e. a move) is equivalent to a copy (this is mandated in the standard)

int val(std::move(*p));

---

Yes, this is still necessary.

delete p;// i think this is no longer necessary. is it ?

Are my move constructor and move assignment operator written well?

Your code is correct.

But there is also a pattern to implement move assignments using std::swap:

Data& operator=(Data&& _d)
{
    std::swap(data, _d.data);
    std::swap(s, _d.size);
    return *this;
}

Here the old data will be released when _d destructor is invoked.

This is not a "more correct way" but it is easier to write and leaves less space to forget to properly dispose the resources.

Move constructor can reuse move assignment:

Data(Data&& _d)
    : this() 
{
    *this = std::move(_d);
}

Again, this is not necessary a better way but reduces amount of duplicated code.

Compare the habits between move and smart pointer in C++?

Always write move-constructor/move-operator= is boring

You almost never need to write your own move constructor/assignment, because (as you mentioned) C++ supplies you with a number of basic resource managers - smart pointers, containers, smart locks etc.

By relying on those in your class you enable default move operations and that results in minimal code size as well as proper semantics:

class MoveClass {
private:
    std::vector<int> data;
public:
    void DoSomething() { /*...*/ }
};

Now you can use your class as in (1) or as a member in other classes, you can be sure that it has move semantics and you did it in the minimal possible amount of code.

The point is one usually only needs to implement move operations for the most low-level classes which are probably covered already by STL, or if some weird specific behavior is needed - both cases should be really rare and not result in "Always writing move-constructor/move-operator=".

Also notice that while approach (1) is unnecessarily verbose, (2) is just unacceptable - you have a resource managing class that doesn't do its job and as a result you have to wrap it in smart pointers everywhere in your code, making it harder to understand and eventually resulting in even more code than (1)

---

Related Topics