C ++ language assignment operator: do I want to use std :: swap with POD types?
Since C ++ 11 when using the jump assignment operator should I have std::swap
all my data including POD types? I guess it doesn't matter for the example below, but I would like to know what the generally accepted best practice is.
Sample code:
class a
{
double* m_d;
unsigned int n;
public:
/// Another question: Should this be a const reference return?
const a& operator=(a&& other)
{
std::swap(m_d, other.m_d); /// correct
std::swap(n, other.n); /// correct ?
/// or
// n = other.n;
// other.n = 0;
}
}
You may like the constructor of the form: - that is: there are always "significant" or specific values stored in n
or m_d
.
a() : m_d(nullptr), n(0)
{
}
source to share
should i std :: swap all my data
Not at all. Moving semantics happens there to speed things up, and exchanging data that is stored directly in objects will usually be slower than copying, and perhaps assign some value to some of the moved data items.
For your specific scenario ...
class a
{
double* m_d;
unsigned int n;
... it's not enough to consider only data members to know what makes sense. For example, if you use your postulated swap combination for non-POD members and assignment otherwise ...
std::swap(m_d, other.m_d);
n = other.n;
other.n = 0;
... in a move constructor or an assignment constructor, it can still invalidate the state of your program by saying that the destructor missed deletion m_d
when it n
was 0
, or if it checked n == 0
before overwriting m_d
with a pointer to newly allocated memory, old memory could leak. You must decide on the class invariants: valid relations m_d
andn
to make sure that your move constructor and / or assignment operator leaves the state valid for future operations. (More often than not, the deleted float might be the only thing to do, but it is valid for the program to reuse the float, for example by assigning a new value to it and working on it in the next loop iteration ....)
Separately, if your invariants admit not nullptr
m_d
while n == 0
, then replacement is m_d
attractive because it gives the moved object permanent control over whatever buffer the moved object may have: it can save time by allocating the buffer later; balancing this pro, if the buffer isn't needed later, you've kept it longer than necessary, and if it's not big enough, you end up deleting and running the larger buffer, but at least you're lazy about that which tends to help performance (but a profile if you need to care).
source to share
I think it should be rewritten this way.
class a
{
public:
a& operator=(a&& other)
{
delete this->m_d; // avoid leaking
this->m_d = other.m_d;
other.m_d = nullptr;
this->n = other.n;
other.n = 0; // n may represents array size
return *this;
}
private:
double* m_d;
unsigned int n;
};
source to share
No, if performance isn't a concern, don't change your POD. There is no benefit over the usual appointment, it just leads to unnecessary copies. Also consider whether you want to set the POD move setting to 0 at all.
I wouldn't even change the pointer. If it's an owner relationship, use unique_ptr and go from it, otherwise treat it the same as POD (copy it and set it to nullptr or whatever your programming logic requires).
If you don't need to set POD to zero and you are using smart pointers, you don't even need to execute your move statement at all.
Regarding the second part of your question: As Mateyush already pointed out, the assignment operator should always return a normal (non-const) reference.
source to share