Writing order and move assignment when following the Rule of Five

I am a C++ newbie, and trying to figure out what the best practices are for writing the "Big Five" member functions, as prescribed by C++11's unofficial "Rule of Five".

My class with a raw pointer is:

```
class MemoryBlock
{
public:
// Default constructor
explicit MemoryBlock(): _length{0}, _data{nullptr} {}
explicit MemoryBlock(const int l): _length{l}, _data{new int[l]} {}

// Big-Five
// --------
// Best Practices?:
// (Copy Constructor)->(Copy Assignment)->(Move Assignment)->(Move Constructor)->(Destructor)
// See the following for more detailed explanations

// Copy constructor
// This approach follows three steps
// (1) Initialize all non-pointer data member with the rhs data, easy!
// (2) Initialize all pointer data member with appropriate new.
// (3) Deep copy all pointers, by either looping thru, or memcopy
MemoryBlock(const MemoryBlock& rhs)
: _length{rhs._length},
_data{new int[rhs._length]}
{
// for (int i = 0; i < rhs._length; ++i)
// {
// _data[i] = rhs._data[i];
// }
//
// Using std::copy is better?
std::copy(rhs._data, rhs._data + _length, _data);
}

// Copy assignment
MemoryBlock& operator=(const MemoryBlock& rhs)
{
// This is the 1st approach
// Compare this pointer, i.e., address same or not!!
// if (this != &rhs)
// {
// delete [] _data;

// _length = rhs._length;
// _data = new int[rhs._data];
// std::copy(rhs._data, rhs._data + _length, _data);
// }

// This is the 2nd approach, a better approach
// This approach follows three steps
// (1) Make sure this is not &rhs
// (2) Create an new instance from rhs by calling copy constructor
// (3) Swap this instance with *this
if (this != &rhs)
{
MemoryBlockVA copy = rhs;
std::swap(copy

Copy Assignment

Checking for self assignment in copy assignment.

if (this != &rhs)
    {
        MemoryBlockVA copy = rhs;
        std::swap(copy, *this);
    }

Sure you can do it. But is it worth it?

The standard answer is no. Even though when it happens this will save you a lot of time (because that copy is a very expensive operation).

Unfortunately self assignment is so exceedingly rare (it barely ever happens) that what you have done is pessimize the co the normal code. So for billions of assignment operations you are making the assignment take slightly longer micro seconds. Then for one operation you are making the code much faster (milli seconds). Is it worth it. Probably not.
Move Assignment

The standard approach is what you commented out swapping

// std::swap(_length, rhs._length);
   // std::swap(_data, rhs._data);

Though I would have implemented a swap method and then written it as:

rhs.swap(*this);

The other thing you should do with Move Assignment is mark the function as non throwing. This allows optimizations when you use your class with the standard library that are not available otherwise.

So I would have written this:

void MemoryBlock::swap(MemoryBlock& other) noexcept
{
    using std::swap;
    swap(_length,   other._length);
    swap(_data,     other._data);
}
friend void swap(MemoryBlock& lhs, MemoryBlock& rhs)
{
    lhs.swap(rhs);
}
MemoryBlock& operator=(MemoryBlock&& rhs) noexcept
{
    rhs.swap(*this);
}

Your implementation has a couple of issues:

Again that check for self assignment that basically never happens (but you have to make work for that billion to one off chance).

if (this != &rhs)
    {
        delete [] _data;

        _length = rhs._length;
        _data = rhs._data;
        rhs._length = 0;
        rhs._data = nullptr;
    }

There is a very expensive call to delete that you may not need to do. Just swap the content to the other object. The semantics of a move put the moved object in an unknown but valid state. If you provide methods that allow you to reset the state to a known value then you can re-use the already allocated memory.

If the other object is going out of scope it will call delete its own destructor. But if it is not going out of scope then you can re-use the already allocated block sometimes.

Also some situations the call to delete is not exception safe. If your pointer is to an unknown type T you can not assume that the destructor of T is exception safe (it should be but it is not required). As a result you would not be able to mark your move operator as noexcept which will then cause performance degradation when you use your object with a standard container.
Move Constructor

Yes this works perfectly:

*this = std::move(rhs); // Assuming you initialize your members in the initializer list first.

But it is also exactly the same as:

rhs.swap(*this);

Which in my mind expresses intent much more precisely.

And again mark your move constructor as noexcept when you can to allow optimizations with the standard container library to be enabled.

MemoryBlock(MemoryBlock&& copy) noexcept
    : _length(0)
    , _data(nullptr)
{
    rhs.swap(*this);
}

Loki's Implementation of MemoryBlock:

• Don't write useless comments (like Default constructor).

• Use default value for the default constructor.

• Don't use leading _ in identifier names (the rules are complex you don't know them all and you will eventually get it wrong. Best to avoid.

.

class MemoryBlock
{
public:
    explicit MemoryBlock(const int l = 0)
        : size{l}
        , data{new int[l]}
    {}

    MemoryBlock(MemoryBlock const& rhs)
        : size{rhs.size}
        , data{new int[rhs.size]}
    {
        std::copy(rhs.data, rhs.data + size, data);
    }    
    MemoryBlock& operator=(MemoryBlock const& rhs)
    {
        MemoryBlock copy(rhs);
        copy.swap(*this);
        return *this;
    }

    MemoryBlock(MemoryBlock&& rhs) noexcept
        : size{0}
        , data{nullptr}
    {
        rhs.swap(*this);
    }
    MemoryBlock& operator=(MemoryBlock&& rhs) noexcept
    {
        rhs.swap(*this);
        return *this;
    }

    void swap(MemoryBlock& rhs) noexcept
    {
        using std::swap;
        swap(size, rhs.size);
        swap(data, rhs.data);
    }
    friend void swap(MemoryBlock& lhs, MemoryBlock& rhs)
    {
        rhs.swap(lhs);
    }

    ~MemoryBlock()
    {
        delete [] data;
    }

    int length() const
    {
        return size;
    }

private:
    int  size;
    int* data;
};

// OR

using MemoryBlock = std::vector;

StackExchange Code Review Q#151760, answer score: 5