Does it make sense to reuse destructor logic by using std::swap in a move assignment operator?

Question

Consider the following:

class Example : boost::noncopyable
{
    HANDLE hExample;
public:
    Example()
    {
        hExample = InitializeHandle();
    }
    ~Example()
    {
        if (hExample == INVALID_HANDLE_VALUE)
        {
            return;
        }
        FreeHandle(hExample);
    }
    Example(Example && other)
        : hExample(other.hExample)
    {
        other.hExample = INVALID_HANDLE_VALUE;
    }
    Example& operator=(Example &&other)
    {
        std::swap(hExample, other.hExample); //?
        return *this;
    }
};

My thinking here is that the destructor will be running on "other" shortly, and as such I don't have to implement my destructor logic again in the move assignment operator by using swap. But I'm not sure that's a reasonable assumption. Would this be "okay"?

See this answer to [Why do some people use swap for move assignments](http://stackoverflow.com/a/6687520/597607) — Bo Persson, Mar 17 '12 at 08:15

score 8 · Answer 1 · edited May 23 '17 at 10:34

8

Imagine the following:

// global variables
Example foo;

struct bar {
    void f() {
        x = std::move(foo); // the old x will now live forever
    }
    Example x;
}

A similar idiom, copy-and-swap (or in this case, move-and-swap), ensures that the destructor is run immediately, which I believe is a better semantic.

Example& operator=(Example other) // other will be moved here
{
    std::swap(hExample, other.hExample);
    return *this;
} // and destroyed here, after swapping

edited May 23 '17 at 10:34

Community

1
1

answered Mar 17 '12 at 02:37

R. Martinho Fernandes

228,013
71
433
510

Wouldn't this allow someone to accidentally "move" away the object they're working with? This object is not copyable because I don't have a good way of copying the "handle" -- which is why I'm supporting move only. – Billy ONeal Mar 17 '12 at 02:41
2

If the object is not movable a compiler error will be generated when the compiler attempts a copy. – R. Martinho Fernandes Mar 17 '12 at 02:42
Sorry, I meant "If the object is not **copyable**" in the last comment. – R. Martinho Fernandes Mar 17 '12 at 02:48
@BillyONeal: The C++ standards committee introduced 3 new kinds of values and all manor of special language to ensure that it is *impossible* to accidentally move from anything that isn't a temporary (or member thereof). So if it's not a temporary, you need to use `std::move` to move from it. – Nicol Bolas Mar 17 '12 at 03:01
@R.MartinhoFernandes: My question is this. The spec talks of copy assignment operators and move assignment operators. Is taking the parameter by value here considered a copy or move assignment operator? Or is it both? Which assignment operator will not be implicitly generated by the compiler by defining the value copy assignment? – Nicol Bolas Mar 17 '12 at 03:04
@Nicol : "*Is taking the parameter by value here considered a copy or move assignment operator?*" It's neither -- a copy or move _constructor_ is invoked. ;-] But assuming that's what you meant, it's dependent on the caller; if an lvalue is passed then it's a copy, otherwise it's a move (and this is why using pass-by-value like this is perfectly okay for non-copyable types). "*Which assignment operator will not be implicitly generated by the compiler by defining the value copy assignment?*" Neither. – ildjarn Mar 17 '12 at 03:07
@ildjarn: The compiler defines the concept of move/copy assignment operators. Classes with user-defined versions of these are different from those without (for example, they are non-trivial). Does this count as a move or copy assignment operator for those purposes? And if not, does this mean that the compiler will still automatically generate one of those operators? – Nicol Bolas Mar 17 '12 at 03:09
1

@Nicol : Ah, I (very much) misunderstood. This counts as both copy and move assignment operator; if one were to try to define either in addition to this, any calls would be ambiguous. – ildjarn Mar 17 '12 at 03:09
@Nicol: See §12.8 paragraph 17: A user-declared copy assignment operator X::operator= is a non-static non-template member function of class `X` with exactly one parameter type `X`, `X&`, `const X&`, `volatile X&` `const volatile X&`. It counts as a copy assignment operator, which prevents generation of the implicit ones. (Though I have to agree that it's weird that it doesn't count as a move assignment operator...) – R. Martinho Fernandes Mar 17 '12 at 03:34
@R.MartinhoFernandes : Defining a copy assignment operator inhibits the generation of a move assignment operator, and since the copy constructor in this case can take an rvalue and move it, it's basically the same difference. The only real downside is the effect on code relying on `std::is_move_constructible<>`. – ildjarn Mar 17 '12 at 03:53
1

@ildjarn `std::is_move_assignable` tests if `x = y` where `y` is an rvalue reference is valid, so that's not a problem. I think really there's nothing this affects. It's just weird that technically you can create a move-only class with a copy assignment operator and no move assignment operator, *and* assignment will have the proper move semantics, not copy. – R. Martinho Fernandes Mar 17 '12 at 04:01
@R.MartinhoFernandes : (s/assignable/constructible/; typo on my part, thanks.) I assumed that `std::is_move_assignable` tested for the actual presence of a move assignment operator; if it deals only with _semantic_ correctness, that's actually good news (and new news to me). :-] – ildjarn Mar 17 '12 at 04:06
@R.MartinhoFernandes OK, this stuff is way too good for comments, so [I asked a question that you can answer.](http://stackoverflow.com/questions/9747406/ramification-of-assignment-operators-with-values-instead-of-references) – Nicol Bolas Mar 17 '12 at 05:00

score 5 · Accepted Answer · edited May 23 '17 at 12:34

5

It should be ok, but it's scarcely any better than the recommended technique of pass-by-value, in which case the move constructor would be used in this situation.

edited May 23 '17 at 12:34

Community

1
1

answered Mar 17 '12 at 02:37

Ben Voigt

277,958
43
419
720

The copy and swap idiom is still normal for move assignment operators? – Billy ONeal Mar 17 '12 at 02:40
@Billy: Yes, you get both move and copy operators for free with copy-and-swap. – Puppy Mar 17 '12 at 02:41
1

@DeadMG: "Free" is a relative term. You get a copy-and-move for copy assignment. So if the object isn't really "moveable" (just regular data without pointers), it costs as much as two copies. – Nicol Bolas Mar 17 '12 at 03:06
@Nicol: One copy is to initialize the parameter. Where's the other? – Ben Voigt Mar 17 '12 at 03:25
@ildjarn: Well, you don't implement your operators using copy-and-swap if you don't have an optimized `swap`. I thought that was obvious. In addition, the FAQ I linked to about copy-and-swap calls out that you MUST redefine `swap` (else you'd get infinite recursion). – Ben Voigt Mar 18 '12 at 03:05
@Ben : Nicol specifically was talking about objects that "aren't really 'moveable' (just regular data without pointers)" -- what is an optimized swap in this scenario? – ildjarn Mar 18 '12 at 17:57
@ildjarn: Objects that aren't really moveable are off-topic for this question, don't you think? But no, such types shouldn't use copy-and-swap, they should just use the compiler-provided default copy. – Ben Voigt Mar 18 '12 at 18:38
@Ben : Maybe, but nonetheless it's what Nicol was referring to and what you responded to. ;-] – ildjarn Mar 18 '12 at 20:09
@ildjarn: Well, the library-provided `std::swap` has three copies. I still don't know where the number two is coming from. – Ben Voigt Mar 18 '12 at 20:34

score 3 · Answer 3 · answered Mar 17 '12 at 02:36

3

My thinking here is that the destructor will be running on "other" shortly

Then your thinking is flawed. You can move from any object you have non-const access to. And the object can continue to live indefinitely after this.

It is technically correct to put your current data in the old object. But it's not a good idea. It's better to use a stack variable:

Example& operator=(Example &&other)
{
    Example temp(std::move(other));  //other is now empty.
    std::swap(hExample, temp);       //our stuff is in `temp`, and will be destroyed
    return *thisl
}

Or better yet (if you're not using Visual Studio) store your stuff in a wrapper that supports movement correctly, and let the compiler-generated move constructor do the job for you.

answered Mar 17 '12 at 02:36

Nicol Bolas

449,505
63
781
982

This is silly. If you want a copy, use pass-by-value which makes the copy (but allows additional optimization opportunities). And doesn't proliferate implementations depending on whether the operand is an xvalue or not. – Ben Voigt Mar 17 '12 at 02:38
@Ben: But if you use pass by value, then someone would be able to pass an lvalue into the operator (it becomes a copy assignment operator) instead. This object is noncopyable. – Billy ONeal Mar 17 '12 at 02:42
@BenVoigt: Even if it wasn't non-copyable, a copy would be it's own separate thing. His situation needs move semantics anyways since the class manages a resource. – Ken Wayne VanderLinde Mar 17 '12 at 02:43
@Billy: If there's no copy constructor, then pass by value will not accept an lvalue. – Ben Voigt Mar 17 '12 at 02:46
+1 I agree with this sentiment. If I have a large `std::vector<>` that I'm moving into, I expect that memory to be returned to me upon moving, not to linger around as long as the object I'm moving _from_ (which may be the scope of a long-running function). – ildjarn Mar 17 '12 at 02:47
@ildjarn: A pass-by-value argument will be destroyed in a timely fashion also. – Ben Voigt Mar 17 '12 at 02:48
@Ben: Ok, how do I force no copy constructor? The typical trick of declaring it `private` isn't working here. (I'm trying to put instances of this noncopyable object into a `vector`, and the `vector` creation is failing) – Billy ONeal Mar 17 '12 at 02:55
@Billy: You inherited from `boost::noncopyable`, didn't that do the trick? – Ben Voigt Mar 17 '12 at 02:56
@Ben: No. Compilation fails deep in the innards of vector; basically saying it's trying to copy, which it can't. – Billy ONeal Mar 17 '12 at 02:59
@Billy : Define the copy constructor with `= delete`. Quoting N3337 §8.4.3/3: "*One can make a class uncopyable, i.e. move-only, by using deleted definitions of the copy constructor and copy assignment operator, and then providing defaulted definitions of the move constructor and move assignment operator.*" – ildjarn Mar 17 '12 at 03:01
@ildjarn: No, VC10 does not support that. – Billy ONeal Mar 17 '12 at 03:11
@Ben: Found the problem; somewhere someone was making a copy of the vector (which understandably was trying to copy the elements). – Billy ONeal Mar 17 '12 at 03:11

Does it make sense to reuse destructor logic by using std::swap in a move assignment operator?

3 Answers3

Linked