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

#ifndef SMART_PTR_H_ #define SMART_PTR_H_ #include <cstdint> template<typename T> class SmartPtr { private: struct Ptr { T* p_; uint64_t count_; Ptr(T* p) : p_{p}, count_{1} {} ~Ptr() { delete p_; } }; public: SmartPtr(T* p) : ptr_{new Ptr{p}} {} ~SmartPtr(); SmartPtr(const SmartPtr<T>& rhs); SmartPtr(SmartPtr<T>&& rhs) =delete; SmartPtr<T>& operator=(const SmartPtr<T>& rhs); SmartPtr<T>& operator=(SmartPtr<T>&& rhs) =delete; T& operator*() { return *ptr_->p_; } T* operator->() { return ptr_->p_; } uint64_t Count() const { return ptr_->count_; } const T* Raw() const { return ptr_->p_; } private: Ptr* ptr_; }; template<typename T> SmartPtr<T>::~SmartPtr() { if (!--ptr_->count_) { delete ptr_; } ptr_ = nullptr; } template<typename T> SmartPtr<T>::SmartPtr(const SmartPtr<T>& rhs) : ptr_{rhs.ptr_} { ++ptr_->count_; } template<typename T> SmartPtr<T>& SmartPtr<T>::operator=(const SmartPtr<T>& rhs) { if (this != &rhs) { if (!--ptr_->count_) { delete ptr_; } ptr_ = rhs.ptr_; ++ptr_->count_; } return *this; } #endif // SMART_PTR_H_

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).

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.
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.

Recommended topics

Hot tags