Writing order and move assignment when following the Rule of Five

Question

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, *this);
 }
 return *this;
 }
 // Move assignment
 MemoryBlock& operator=(MemoryBlock&& rhs)
 {
 // This 1st approach follows four steps
 // (1) Make sure this is not &rhs
 // (2) Reclaim original pointer data resources by delete
 // (3) Shallow copy all data member, including pointers
 // (4) Set rhs back to nullptr or zeros
 if (this != &rhs)
 {
 delete [] _data;
 _length = rhs._length;
 _data = rhs._data;
 rhs._length = 0;
 rhs._data = nullptr;
 }
 // This 2nd approach is given by a textbook
 // std::swap(_length, rhs._length);
 // std::swap(_data, rhs._data);
 return *this;
 }
 // Move constructor
 MemoryBlock(MemoryBlock&& rhs)
 : _length{0},
 _data{nullptr}
 {
 // This is the 1st approach
 // _length = rhs._length;
 // _data = rhs._data;
 // rhs._length = 0;
 // rhs._data = nullptr;
 // This is the 2nd approach, a better and smarter approach!
 *this = std::move(rhs); 
 }
 ~MemoryBlock()
 {
 // Only need to take care of the pointer data member, to reclaim them.
 if (_data != nullptr)
 {
 delete [] _data;
 }
 }
 int length() const
 {
 return _length;
 }
private:
 int _length;
 int* _data;
};

First, please feel free to suggest places for improvement in this code. I gathered ideas here and there, from websites and textbooks, and then took notes myself.

But my specific questions are:

1. Are there any potential problems with this approach? I mean following the order of Copy Constructor → Copy Assignment → Move Assignment → Move Constructor → Destructor. I'm thinking that "Copy Assignment" can be easily and safely written using the "copy and swap" idiom based on the existing "Copy Constructor", and "Move Constructor" can be easily and safely written based on "Move Assignment".
2. In writing assignment operators, should we always check and see if if (this != &rhs)?
3. In writing the Move Assignment, is the 2nd approach for "doing swaps" better than the 1st one?

By "better", I mean that it has the same effect without sacrificing safety.

Here is a small working example using this class:

#include "MemoryBlock.hpp"
int main (int argc, const char* argv [])
{
 MemoryBlock x(10000);
 std::cout<< "x's length = " << x.length() << std::endl;
 return 0;
}

• Mast: For those among us who don't have a clue what a Big Five is (I thought they were animals), could you add a description of what problem your code solves?
• Incomputable: @Mast, I believe he is talking about rule of five.
• Incomputable: @Daniel, could you post an example usage of the code? Small compilable and runnable example would be perfect.
• Daniel: @Incomputable, Added now. thank you, yes, I mean rule of five.

Answer 1

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:

1. Don't write useless comments (like Default constructor).
2. Use default value for the default constructor.
3. 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<int>;

• Daniel: Hi Loki, thank you so much. May I ask a question, maybe this is a stupid question. But if these best practices prove to be a better one, why wonldn't the compiler generated them for the programmers? Because originally I thought, oh, raw pointers are tricky, programmers have to handle themselves. But now I saw that even class with one or more raw pointers can have some best practices rules to follow. If so, why not generated the codes for the users automatically. Is there anything, any complex situation, I have overlooked?
• Loki Astari: @Daniel Unfortunately C++ inherited its semantics from C. One of the biggests principles of updating C++ is a strict adherence to backwards compatibility and update to the language should not break old code. Note if you class has more than one "owned" raw pointer you are probably doing it wrong. Every pointer should be wrapped in its own class to manage it's lifespan. Now normally most of these occurrences of "owned" raw pointer are covered by either std::<container> or std::<smart_pointer> so there is little need to do it yourself unless implementing a new container/smart_pointer.
• Daniel: Reading isocpp.github.io/CppCoreGuidelines. "C.64: A move operation should move and leave its source in a valid state" In your move ctor, without setting rhs.length = 0; and rhs.data = nullptr;, would that create some problems in the future?
• Daniel: Also according to that link (C.44: Prefer default constructors to be simple and non-throwing), do you think adding MemoryBlock() noexcept {} would make this code even better? And also considering C45 and C48, do you think the following would be better? First:private: int length {0}; int* data {nullptr}; and then write the constructor as explicit MemoryBlock(int l) : length{l} , data{new int[l]} {}? Thanks
• Daniel: My apologies for so many questions, but what the reason for adding that friend void swap function? Thank you so much.