Questions about the move assignment operator
Imagine the following class managing a resource (my question only concerns the assignment operator):
struct A
{
std::size_t s;
int* p;
A(std::size_t s) : s(s), p(new int[s]){}
~A(){delete [] p;}
A(A const& other) : s(other.s), p(new int[other.s])
{std::copy(other.p, other.p + s, this->p);}
A(A&& other) : s(other.s), p(other.p)
{other.s = 0; other.p = nullptr;}
A& operator=(A const& other)
{A temp = other; std::swap(*this, temp); return *this;}
// Move assignment operator #1
A& operator=(A&& other)
{
std::swap(this->s, other.s);
std::swap(this->p, other.p);
return *this;
}
// Move assignment operator #2
A& operator=(A&& other)
{
delete [] p;
s = other.s;
p = other.p;
other.s = 0;
other.p = nullptr;
return *this;
}
};
Question:
What are the advantages and disadvantages of the two assignment operators # 1 and # 2 above? I believe the only difference I can see is what std::swap
the lhs store saves, however I don't see how that would be useful since the rvalues will be destroyed anyway. Maybe the only time will be with something like a1 = std::move(a2);
, but even so I see no reason to use # 1.
source to share
This is where you should really measure.
And I look at the copy assignment operator and see inefficiency:
A& operator=(A const& other)
{A temp = other; std::swap(*this, temp); return *this;}
What if *this
u other
have the same s
?
It seems to me that a smarter copy assignment might avoid the trip to the heap if s == other.s
. All you need to do is a copy:
A& operator=(A const& other)
{
if (this != &other)
{
if (s != other.s)
{
delete [] p;
p = nullptr;
s = 0;
p = new int[other.s];
s = other.s;
}
std::copy(other.p, other.p + s, this->p);
}
return *this;
}
If you do not need a strong defense of exceptions, only the basic exception safety when the copy destination (eg std::string
, std::vector
etc.), that is a possible increase in productivity with the foregoing. How many? Measure.
I have coded this class in three ways:
Design 1:
Use the above copy assignment operator and OP # 1 carry assignment operator.
Design 2:
Use the above copy assignment operator and OP # 2 assignment operator.
Design 3:
DeadMG copy destination operator for copy and move destination.
Here's the code I used for testing:
#include <cstddef>
#include <algorithm>
#include <chrono>
#include <iostream>
struct A
{
std::size_t s;
int* p;
A(std::size_t s) : s(s), p(new int[s]){}
~A(){delete [] p;}
A(A const& other) : s(other.s), p(new int[other.s])
{std::copy(other.p, other.p + s, this->p);}
A(A&& other) : s(other.s), p(other.p)
{other.s = 0; other.p = nullptr;}
void swap(A& other)
{std::swap(s, other.s); std::swap(p, other.p);}
#if DESIGN != 3
A& operator=(A const& other)
{
if (this != &other)
{
if (s != other.s)
{
delete [] p;
p = nullptr;
s = 0;
p = new int[other.s];
s = other.s;
}
std::copy(other.p, other.p + s, this->p);
}
return *this;
}
#endif
#if DESIGN == 1
// Move assignment operator #1
A& operator=(A&& other)
{
swap(other);
return *this;
}
#elif DESIGN == 2
// Move assignment operator #2
A& operator=(A&& other)
{
delete [] p;
s = other.s;
p = other.p;
other.s = 0;
other.p = nullptr;
return *this;
}
#elif DESIGN == 3
A& operator=(A other)
{
swap(other);
return *this;
}
#endif
};
int main()
{
typedef std::chrono::high_resolution_clock Clock;
typedef std::chrono::duration<float, std::nano> NS;
A a1(10);
A a2(10);
auto t0 = Clock::now();
a2 = a1;
auto t1 = Clock::now();
std::cout << "copy takes " << NS(t1-t0).count() << "ns\n";
t0 = Clock::now();
a2 = std::move(a1);
t1 = Clock::now();
std::cout << "move takes " << NS(t1-t0).count() << "ns\n";
}
Here is the result I got:
$ clang++ -std=c++11 -stdlib=libc++ -O3 -DDESIGN=1 test.cpp
$ a.out
copy takes 55ns
move takes 44ns
$ a.out
copy takes 56ns
move takes 24ns
$ a.out
copy takes 53ns
move takes 25ns
$ clang++ -std=c++11 -stdlib=libc++ -O3 -DDESIGN=2 test.cpp
$ a.out
copy takes 74ns
move takes 538ns
$ a.out
copy takes 59ns
move takes 491ns
$ a.out
copy takes 61ns
move takes 510ns
$ clang++ -std=c++11 -stdlib=libc++ -O3 -DDESIGN=3 test.cpp
$ a.out
copy takes 666ns
move takes 304ns
$ a.out
copy takes 603ns
move takes 446ns
$ a.out
copy takes 619ns
move takes 317ns
DESIGN 1
looks pretty good to me.
Warning. If the class has resources that need to be released quickly, such as a mutex lock or ownership of an open source file, the project move assignment operator-2 might be better in terms of correctness. But when the resource is just memory, it is often beneficial to delay freeing it as long as possible (as in the OP's use case).
Caveat 2: If you have other use cases that you know to be important, measure them. You may come to different conclusions than here.
Note. I rate the performance above "DRY". All the code here will be encapsulated in one class ( struct A
). Do struct A
as well as you can. And if you do good enough work, your clients from struct A
(who might be themselves) won't be tempted by "RIA" (Reinvent It Again). I prefer to repeat a bit of code within a single class rather than repeating the implementation of entire classes over and over again.
source to share
It is more correct to use # 1 than # 2, because if you use # 2, you are breaking DRY and duplicating your destructor logic. Second, consider the following assignment operator:
A& operator=(A other) {
swap(*this, other);
return *this;
}
These are copy and move assignment operators without duplicate code - great form.
source to share
The assignment statement posted by DeadMG does things right if swap()
ing objects cannot be thrown away. Unfortunately, this cannot always be guaranteed! Particularly if you have controllers with set states and that won't work. If the allocators might be different, you feel like you need separate copy and move: the copy constructor will unconditionally create the copy passed in the allocator:
T& T::operator=(T const& other) {
T(other, this->get_allocator()).swap(*this);
return * this;
}
The branch assignments will be checked if the allocators are identical, and if so, just swap()
two objects and otherwise just call the copy assignment:
T& operator= (T&& other) {
if (this->get_allocator() == other.get_allocator()) {
this->swap(other);
}
else {
*this = other;
}
return *this;
}
The version taking value is a much simpler alternative that should be preferred if noexcept(v.swap(*this))
- true
.
This implicitly also responds to the original qurstion: when throwing swap()
and moving the destination, both implementations are wrong, since they are not the main exception. Assuming that the only source of exceptions in swap()
are mismatched allocators, the implementation above is a safe exception.
source to share