A better way to implement copy-and-swap idiom in C++11

Question

I saw many code implementing rule of five in terms of copy and swap, but I think we can use a move function to replace the swap function as in the following code:

#include <algorithm>
#include <cstddef>

class DumbArray {
public:
    DumbArray(std::size_t size = 0)
        : size_(size), array_(size_ ? new int[size_]() : nullptr) {
    }

    DumbArray(const DumbArray& that)
        : size_(that.size_), array_(size_ ? new int[size_] : nullptr) {
        std::copy(that.array_, that.array_ + size_, array_);
    }

    DumbArray(DumbArray&& that) : DumbArray() {
        move_to_this(that);
    }

    ~DumbArray() {
        delete [] array_;
    }

    DumbArray& operator=(DumbArray that) {
        move_to_this(that);
        return *this;
    }

private:
    void move_to_this(DumbArray &that) {
        delete [] array_;
        array_ = that.array_;
        size_ = that.size_;
        that.array_ = nullptr;
        that.size_ = 0;
   }

private:
    std::size_t size_;
    int* array_;
};

This code, I think

1. Exception safe
2. Require less typing, as many function just call move_to_this(), and copy assignment and move assignment are unified in one single function
3. More efficient than copy-and-swap, as swap involves 3 assignments, while here just 2, and this code doesn't suffer the problems mentioned in This Link

Am I right?

Thanks

Edit:

1. As @Leon pointed out, maybe a dedicated function for freeing resource is needed, to avoid code duplication in move_to_this() and destructor

2. As @thorsan pointed out, for extreme performance concern, it's better to seperate DumbArray& operator=(DumbArray that) { move_to_this(that); return *this; } into DumbArray& operator=(const DumbArray &that) { DumbArray temp(that); move_to_this(temp); return *this; }(thanks to @MikeMB) and DumbArray& operator=(DumbArray &&that) { move_to_this(that); return *this; } to avoid an extra move operatoin

   After adding some debug print, I found that no extra move is involved in DumbArray& operator=(DumbArray that) {} when you call it as a move assignment

3. As @Erik Alapää pointed out, a self-assignment check is needed before delete in move_to_this()

Answer 1

comments inline, but briefly:

• you want all move assignments and move constructors to be noexcept if at all possible. The standard library is much faster if you enable this, because it can elide any exception handling from algorithms which reorder a sequence of your object.

• if you're going to define a custom destructor, make it noexcept. Why open pandora's box? I was wrong about this. It's noexcept by default.

• In this case, providing the strong exception guarantee is painless and costs almost nothing, so let's do that.

code:

#include <algorithm>
#include <cstddef>

class DumbArray {
public:
    DumbArray(std::size_t size = 0)
    : size_(size), array_(size_ ? new int[size_]() : nullptr) {
    }

    DumbArray(const DumbArray& that)
    : size_(that.size_), array_(size_ ? new int[size_] : nullptr) {
        std::copy(that.array_, that.array_ + size_, array_);
    }

    // the move constructor becomes the heart of all move operations.
    // note that it is noexcept - this means our object will behave well
    // when contained by a std:: container
    DumbArray(DumbArray&& that) noexcept
    : size_(that.size_)
    , array_(that.array_)
    {
        that.size_ = 0;
        that.array_ = nullptr;
    }

    // noexcept, otherwise all kinds of nasty things can happen
    ~DumbArray() // noexcept - this is implied.
    {
        delete [] array_;
    }

    // I see that you were doing by re-using the assignment operator
    // for copy-assignment and move-assignment but unfortunately
    // that was preventing us from making the move-assignment operator
    // noexcept (see later)
    DumbArray& operator=(const DumbArray& that)
    {
        // copy-swap idiom provides strong exception guarantee for no cost
        DumbArray(that).swap(*this);
        return *this;
    }

    // move-assignment is now noexcept (because move-constructor is noexcept
    // and swap is noexcept) This makes vector manipulations of DumbArray
    // many orders of magnitude faster than they would otherwise be
    // (e.g. insert, partition, sort, etc)
    DumbArray& operator=(DumbArray&& that) noexcept {
        DumbArray(std::move(that)).swap(*this);
        return *this;
    }


    // provide a noexcept swap. It's the heart of all move and copy ops
    // and again, providing it helps std containers and algorithms 
    // to be efficient. Standard idioms exist because they work.
    void swap(DumbArray& that) noexcept {
        std::swap(size_, that.size_);
        std::swap(array_, that.array_);
    }

private:
    std::size_t size_;
    int* array_;
};

There is one further performance improvement one could make in the move-assignment operator.

The solution I have offered provides the guarantee that a moved-from array will be empty (with resources deallocated). This may not be what you want. For example if you tracked the capacity and the size of a DumbArray separately (for example, like std::vector), then you may well want any allocated memory in this to be retained in that after the move. This would then allow that to be assigned to while possibly getting away without another memory allocation.

To enable this optimisation, we simply implement the move-assign operator in terms of (noexcept) swap:

so from this:

    /// @pre that must be in a valid state
    /// @post that is guaranteed to be empty() and not allocated()
    ///
    DumbArray& operator=(DumbArray&& that) noexcept {
        DumbArray(std::move(that)).swap(*this);
        return *this;
    }

to this:

    /// @pre that must be in a valid state
    /// @post that will be in an undefined but valid state
    DumbArray& operator=(DumbArray&& that) noexcept {
        swap(that);
        return *this;
    }

In the case of the DumbArray, it's probably worth using the more relaxed form in practice, but beware of subtle bugs.

e.g.

DumbArray x = { .... };
do_something(std::move(x));

// here: we will get a segfault if we implement the fully destructive
// variant. The optimised variant *may* not crash, it may just do
// something_else with some previously-used data.
// depending on your application, this may be a security risk 

something_else(x);   

Answer 2

The only (small) problem with your code is the duplication of functionality between move_to_this() and the destructor, which is a maintenance issue should your class need to be changed. Of course it can be solved by extracting that part into a common function destroy().

My critique of the "problems" discussed by Scott Meyers in his blog post:

He tries to manually optimize where the compiler could do an equally good job if it is smart enough. The rule-of-five can be reduced to the rule-of-four by

• providing only the copy assignment operator that takes its argument by value and
• not bothering to write the move assignment operator (exactly what you did).

This automatically solves the problem of the resources of the left-hand-side object being swapped into the right-hand-side object and not being immediately released if the right-hand-side object is not a temporary.

Then, inside the implementation of the copy assignment operator according to the copy-and-swap idiom, swap() will take as one of its arguments an expiring object. If the compiler can inline the destructor of the latter, then it will definitely eliminate the extra pointer assignment - indeed, why save the pointer that is going to be deleteed on the next step?

My conclusion is that it is simpler to follow the well established idiom instead of slightly complicating the implementation for the sake of micro-optimizations that are well within the reach of a mature compiler.