3

Since C++11, when using the move assignment operator, should I std::swap all my data, including POD types? I guess it doesn't make a difference for the example below, but I'd like to know what the generally accepted best practice is.

Example 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 might like to consider a constructor of the form: - ie: there are always "meaningful" or defined values stores in n or m_d.

a() : m_d(nullptr), n(0)
{
}
FreelanceConsultant
  • 13,167
  • 27
  • 115
  • 225
  • For your commented question, it should be `a& operator=(a&& other)` – Jarod42 Aug 03 '15 at 11:10
  • You can probably use only `n = other.n`, as the moved-from object should be left in a valid state (the standard doesn't specify the end state). – vsoftco Aug 03 '15 at 11:13
  • 1
    @vsoftco If n is, say, the size of a dynamic array, then it should be set to 0 or `other.n`, depending on what is done with `m_d`. – juanchopanza Aug 03 '15 at 11:14
  • BTW you should probably write a swap function for your class, and use it in the move assignment operator if that is how you want to implement move assignment. – juanchopanza Aug 03 '15 at 11:17
  • @juanchopanza Is this enforced by the standard? AFAIK, we set pointers to `nullptr` so we avoid double deallocation, but why should we set other members to zero?. – vsoftco Aug 03 '15 at 11:17
  • 1
    @vsoftco The standard doesn't enforce anything. You can do whatever you want. I prefer to leave moved-from objects in a self-consistent state. – juanchopanza Aug 03 '15 at 11:21
  • 2
    Why are you implementing your move constructor at all? What about your class would make an implementation provided one inadequate? – CB Bailey Aug 03 '15 at 11:36
  • @CharlesBailey Wouldn't the default one just copy the pointer, leading to two objects "owning" the same resource? – juanchopanza Aug 03 '15 at 11:37
  • 1
    @juanchopanza: If the raw pointer is being used as an "owning" pointer then that would be a reason, yes. (I'd argue that there would be better ways to fix this than by manually implementing a move constructor.) – CB Bailey Aug 03 '15 at 11:40
  • 2
    See [The Drawbacks of Implementing Move Assignment in Terms of Swap](http://scottmeyers.blogspot.co.uk/2014/06/the-drawbacks-of-implementing-move.html) – Chris Drew Aug 03 '15 at 11:44
  • @CharlesBailey Right, I was assuming it was an owning pointer. Hard to say from the posted pseudocode. – juanchopanza Aug 03 '15 at 11:45
  • To implement a move constructor for this class you re-write the class as `class a { std::vector m_d; };` – Chris Drew Aug 03 '15 at 13:41

3 Answers3

2

I think this should be rewriten 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;
};
Richard Dally
  • 1,432
  • 2
  • 21
  • 38
  • 1
    For your `operator=`, you might want to `delete m_d;` before leaking it. And some discussion of what could happen with `other.n` left at its old value could be help readers understand why you *"think this should be rewritten this way"*. – Tony Delroy Aug 03 '15 at 12:37
  • 1
    From [standard](http://stackoverflow.com/a/12095473/5037799): `Unless otherwise specified, such moved-from objects shall be placed in a valid but unspecified state.` There is no valid value for unsigned int to set an unspecified state. – Richard Dally Aug 03 '15 at 13:11
  • 2
    you're misunderstanding that requirement - the *object* needs to be in a valid state, not just the `unsigned int` member in isolation. That means that whatever other operations are attempted on a moved-from instance of `a` must work per the class API: for example, if there's an existing indexing `operator[](int i)` implemented with `assert(i < n); return m_d[n];` your choice to leave `other.n` as is would be undesirable - either an assertion that `m_d != nullptr` should be added, or `other.n set to 0`: without seeing other operations, you can't reason about what's valid in `op=(a&&)` – Tony Delroy Aug 03 '15 at 13:24
  • I understand your point if `m_d` and `n` are related, `n` should be set to zero. – Richard Dally Aug 03 '15 at 13:32
  • 1
    @TonyD The interpretation of the standard that I've heard is that a moved-from object must be left in a destructable state, but not necessarily a fully functional class (as per its API). A class may go beyond that and say "a moved-from vector is equivalent to a default-constructed vector" (as an example). – Andre Kostur Aug 03 '15 at 13:53
  • This will currently not leave the moved-from object in a "valid" state after self-assignment but there is [some debate](http://stackoverflow.com/q/9322174/3422652) if this actually matters. – Chris Drew Aug 03 '15 at 14:31
  • @AndreKostur: destructible is a minimum, but IMHO it's a good idea to make sure they can be assigned to and reused... that's the more defensive choice, and ensures users of the class don't get a nasty surprise when doing something like say remove/erase with move iterators on an array, followed by assigning to moved-from elements... in a case like this where it's simply a matter of assigning to one extra `int` member - with that assignment almost certain to be optimised away if unneeded - I see no reason to take risks. – Tony Delroy Aug 03 '15 at 15:20
1

No, if efficiency is any concern, don't swap PODs. There is just no benefit compared to normal assignment, it just results in unnecessary copies. Also consider if setting the moved from POD to 0 is even required at all.

I wouldn't even swap the pointer. If this is an owning relationship, use unique_ptr and move from it, otherwise treat it just like a POD (copy it and set it to nullptr afterwards or whatever your program logic requires).

If you don't have to set your PODs to zero and you use smart pointers, you don't even have to implement your move operator at all.

Concerning the second part of your question: As Mateusz already stated, the assignment operator should always return a normal (non-const) reference.

MikeMB
  • 20,029
  • 9
  • 57
  • 102
1

should I std::swap all my data

Not generally. Move semantics are there to make things faster, and swapping data that's stored directly in the objects will normally be slower than copying it, and possibly assigning some value to some of the moved-from data members.

For your specific scenario...

class a
{
     double* m_d;
     unsigned int n; 

...it's not enough to consider just the data members to know what makes sense. For example, if you use your postulated combination of swap for non-POD members and assignment otherwise...

     std::swap(m_d, other.m_d);
     n = other.n;
     other.n = 0;

...in the move constructor or assignment operator, then it might still leave your program state invalid if say the destructor skipped deleting m_d when n was 0, or if it checked n == 0 before overwriting m_d with a pointer to newly allocated memory, old memory may be leaked. You have to decide on the class invariants: the valid relationships of m_d and n, to make sure your move constructor and/or assignment operator leave the state valid for future operations. (Most often, the moved-from object's destructor may be the only thing left to run, but it's valid for a program to reuse the moved-from object - e.g. assigning it a new value and working on it in the next iteration of a loop....)

Separately, if your invariants allow a non-nullptr m_d while n == 0, then swapping m_ds is appealing as it gives the moved-from object ongoing control of any buffer the moved-to object may have had: that may save time allocating a buffer later; counter-balancing that pro, if the buffer's not needed later you've kept it allocated longer than necessary, and if it's not big enough you'll end up deleting and newing a larger buffer, but at least you're being lazy about it which tends to help performance (but profile if you have to care).

Tony Delroy
  • 102,968
  • 15
  • 177
  • 252