Is it a good habit to reset self to nullptr when using move-constructor?
in C ++ 11, move / move support move-constructor / operator.
This is my example:
class A {
public:
A() : table_(nullptr), alloc_(0) {}
~A()
{
if (table_)
delete[] table_;
}
A(const A & other)
{
// table_ is not initialized
// if (table_)
// delete[] table_;
table_ = new int[other.alloc_];
memcpy(table_, other.table_, other.alloc_ * sizeof(int));
alloc_ = other.alloc_;
}
A& operator=(const A & other)
{
if (table_)
delete[] table_;
table_ = new int[other.alloc_];
memcpy(table_, other.table_, other.alloc_ * sizeof(int));
alloc_ = other.alloc_;
return *this;
}
A(A && other)
{
// table_ is not initialized in constructor
// if (table_)
// delete[] table_;
table_ = other.table_;
alloc_ = other.alloc_;
}
A& operator=(A && other)
{
if (table_)
delete[] table_;
table_ = other.table_;
alloc_ = other.alloc_;
}
private:
int *table_;
int alloc_;
};
Seems good, but sometimes I want to move a local variable, like:
class B {
private:
A a_;
public:
void hello()
{
A tmp;
// do something to tmp
a_ = std::move(tmp);
// tmp.~A() is called, so a_ is invalid now.
}
};
when the end function is called tmp.~A()
, at that time a_
and tmp
has the same pointer table_
when tmp delete[] table_
, a_ table_
will be invalid.
I am wandering about when should I use std::move
to assign tmp to a_ without copying.
using the answers, I am modifying the move constructor A like this:
class A {
private:
void reset()
{
table_ = nullptr;
alloc_ = 0;
}
public:
A(A && other)
{
table_ = other.table_;
alloc_ = other.alloc_;
other.reset();
}
A& operator=(A && other)
{
std::swap(table_, other.table_);
std::swap(alloc_, other.alloc_);
}
};
In this code, when I move something, I will change the new and old link, so the old one tmp
will remove [] the original a_ table_, which is useless.
It's a good habit to do this.
source to share
When you move from other
to A(A && other)
, you must also set your moved items to nulltpr. Therefore, the fixed code should look like this:
A(A && other)
{
//if (table_)
// delete[] table_; // no need for this in move c-tor
table_ = other.table_;
other.table_ = nullptr;
alloc_ = other.alloc_;
other.alloc_ = nullptr;
}
A& operator=(A && other)
{
// as n.m. has pointed out, this move assignment does not
// protect against self assignment. One solution is to use
// swap aproach here. The other is to simply check if table_ == other.table_.
// Also see here for drawbacks of swap method:
// http://scottmeyers.blogspot.com/2014/06/the-drawbacks-of-implementing-move.html
delete[] table_;
table_ = other.table_;
other.table_ = nullptr;
alloc_ = other.alloc_;
other.alloc_ = nullptr;
return *this;
}
This puts other
in what standard calls valid but unspecified state
.
you can also use std :: swap like this:
A(A && other)
{
table_ = other.table_;
alloc_ = other.alloc_;
}
A& operator=(A && other)
{
std::swap(table_, other.table_);
std::swap(alloc_, other.alloc_);
return *this;
}
thus, the deallocation will be performed when the object is destroyed from the object.
source to share
There are many problems with this code (even if you want to mess with arrays and pointers, which you shouldn't in real life. Just use std :: vector).
Bad code:
A()
{
table_ = nullptr;
alloc_ = 0;
}
Don't use assignment in the ctor corpus, use init-lists members. Nice code:
A() : table{nullptr}, alloc_ {0} {}
Ditto for other constructors.
Backup code:
if (table_)
delete[] table_;
delete
will check your pointer again. delete
ing a is nullptr
completely safe. Do not worry.
VERY bad code:
A(const A & other)
{
if (table_)
delete[] table_;
table
not initialized. Access to it is UB. Moreover, there is no need to do this check in the constructor. There will be no provision for the newly built facility. Just remove the check. The same goes for other constructors.
Bad code:
A& operator=(const A & other)
{
if (table_)
delete[] table_;
Does not protect itself from self-appointment. The same goes for other assignment operators.
These are all habits that need to be ill-mannered, whether it is code for C ++ 03 or C ++ 11. Now to move:
A(A && other)
{
if (table_)
delete[] table_;
table_ = other.table_;
alloc_ = other.alloc_;
}
This is totally wrong. You need to change the object you are moving from , otherwise it is not a movement at all, but a simple shallow copy.
A(A && other) : table_{other.table_}, alloc_{other.alloc_} {
{
other.table_ = nullptr;
other.alloc_ = 0;
}
Ditto for move assignment.
std::swap
in ctor movement is a great idiom when you are dealing with user defined types. This is not entirely necessary for primitive types, mainly because you need to initialize them first and then immediately swap them, but you can still use them.
source to share
Your move constructor and assignment operator do shallow copy efficiently. You must set other.table
to nullptr
for the transition to make sense in this case. And of course, this will avoid undefined behavior when deleting the same array twice, as you might suggest in your example.
source to share
Replacing the values in the move constructor is a good alternative.
A& operator=(A && other)
{
using namespace std;
swap(table_, other.table_);
swap(alloc_, other.alloc_);
return *this;
}
This way, the content of the source is placed on the target, and the last content is moved into the source code, which then cleans them up gracefully on deletion (which is what you expect anyway, otherwise you wouldn't want to move the object ...).
Moving a constructor can benefit from the above assignment:
A(A&& other) : A()
{
*this = std::move(other);
}
source to share
In the move / assign constructor, after copying the pointer, assign them "nullptr" so that when the destructor is called, it will not be op.
This is how I could go and write a move constructor and assignment. Also, you can avoid "if" check "delete", if it's "nullptr" it won't be an op.
A(A && other)
{
delete[] table_;
table_ = other.table_;
other.table_ = nullptr;
alloc_ = other.alloc_;
}
A& operator=(A && other) {
delete[] table_;
table_ = other.table_;
other.table_ = nullptr; // assign the source to be nullptr
alloc_ = other.alloc_;
return *this;
}
source to share
I think you need to change your move mechanism and assignment operator like this:
// Simple move constructor
A(A&& arg) : member(std::move(arg.member)) // the expression "arg.member" is lvalue
{}
// Simple move assignment operator
A& operator=(A&& other) {
member = std::move(other.member);
return *this;
}
as defined here
source to share