Should the Copy-and-Swap Idiom become the Copy-and-Move Idiom in C++11?

Question

As explained in this answer, the copy-and-swap idiom is implemented as follows:

class MyClass
{
private:
    BigClass data;
    UnmovableClass *dataPtr;

public:
    MyClass()
      : data(), dataPtr(new UnmovableClass) { }
    MyClass(const MyClass& other)
      : data(other.data), dataPtr(new UnmovableClass(*other.dataPtr)) { }
    MyClass(MyClass&& other)
      : data(std::move(other.data)), dataPtr(other.dataPtr)
    { other.dataPtr= nullptr; }

    ~MyClass() { delete dataPtr; }

    friend void swap(MyClass& first, MyClass& second)
    {
        using std::swap;
        swap(first.data, other.data);
        swap(first.dataPtr, other.dataPtr);
    }

    MyClass& operator=(MyClass other)
    {
        swap(*this, other);
        return *this;
    }
};

By having a value of MyClass as parameter for operator=, the parameter can be constructed by either the copy constructor or the move constructor. You can then safely extract the data from the parameter. This prevents code duplication and assists in exception safety.

The answer mentions you can either swap or move the variables in the temporary. It primarily discusses swapping. However, a swap, if not optimised by the compiler, involves three move operations, and in more complex cases does additional extra work. When all you want, is to move the temporary into the assigned-to object.

Consider this more complex example, involving the observer pattern. In this example, I've written the assignment operator code manually. Emphasis is on the move constructor, assignment operator and swap method:

class MyClass : Observable::IObserver
{
private:
    std::shared_ptr<Observable> observable;

public:
    MyClass(std::shared_ptr<Observable> observable) : observable(observable){ observable->registerObserver(*this); }
    MyClass(const MyClass& other) : observable(other.observable) { observable.registerObserver(*this); }
    ~MyClass() { if(observable != nullptr) { observable->unregisterObserver(*this); }}

    MyClass(MyClass&& other) : observable(std::move(other.observable))
    {
        observable->unregisterObserver(other);
        other.observable.reset(nullptr);
        observable->registerObserver(*this);
    }

    friend void swap(MyClass& first, MyClass& second)
    {
        //Checks for nullptr and same observable omitted
            using std::swap;
            swap(first.observable, second.observable);

            second.observable->unregisterObserver(first);
            first.observable->registerObserver(first);
            first.observable->unregisterObserver(second);
            second.observable->registerObserver(second);
    }

    MyClass& operator=(MyClass other)
    {
        observable->unregisterObserver(*this);
        observable = std::move(other.observable);

        observable->unregisterObserver(other);
        other.observable.reset(nullptr);
        observable->registerObserver(*this);
    }
}

Clearly, the duplicated part of the code in this manually written assignment operator is identical to that of the move constructor. You could perform a swap in the assignment operator and the behaviour would be right, but it would potentially perform more moves and perform an extra registration (in the swap) and unregistration (in the destructor).

Wouldn't it make much more sense to reuse the move constructor's code in stead?

private:
    void performMoveActions(MyClass&& other)
    {
        observable->unregisterObserver(other);
        other.observable.reset(nullptr);
        observable->registerObserver(*this);
    }

public:
    MyClass(MyClass&& other) : observable(std::move(other.observable))
    {
        performMoveActions(other);
    }

    MyClass& operator=(MyClass other)
    {
        observable->unregisterObserver(*this);
        observable = std::move(other.observable);

        performMoveActions(other);
    }

It looks to me like this approach is never inferior to the swap approach. Am I right in thinking that the copy-and-swap idiom would be better off as the copy-and-move idiom in C++11, or did I miss something important?

Andeee

It should be noted that the default implementation of std::swap will use the move constructor if you don't specialize it yourself, resulting in code that is pretty much equivalent to your second example. – Stigmasterol

@Stigmasterol It will use one move construct and two move-assigns, though. And the move code is already there anyway, for the move constructor, so there doesn't seem to be a need to use a swap in stead of reusing the move constructor code. – Andeee

@Aberrant: show a little faith in your compiler, it's good at eliminating useless stuff. – Fatherinlaw

This question is extremely verbose and could probably be expressed in less than half this length. Could you clean it up? – Alamo

@mydogisbox Good point, I'll try. – Andeee

@mydogisbox I think I've stripped the question to its essentials now. – Andeee

@Andeee much more readable! – Alamo

I've never used the observer pattern, and I haven't analyzed this too closely, so forgive me if I'm wrong, but I think I see two problems: 1.) Your proposed assignment operator does not seem to free the resources currently held by *this. That is, you copy over observable before unregistering *this from the old observable target. 2.) If you did free your resources before copying or moving, you'd have to be careful not to self-assign. You CAN speed up copy-and-swap though: Instead of swapping all members, swap only "resource" members affecting the destructor, and move-assign everything else. – Predicament

@MikeS Well spotted! I've fixed it, thanks for pointing it out. @ Anyone The fact that I made this error, along with the added problem on self-assignment is starting to turn me more sympathetic towards the swap approach. If an answer would explain the fact that the move approach is more error-prone, I may well accept it. Perhaps once I've fully made my mind up about this issue I may write such an answer myself. – Andeee

Answer 1

It's been a long time since I asked this question, and I've known the answer for a while now, but I've put off writing the answer for it. Here it is.

The answer is no. The Copy-and-swap idiom should not become the Copy-and-move idiom.

An important part of Copy-and-swap (which is also Move-construct-and-swap) is a way to implement assignment operators with safe cleanup. The old data is swapped into a copy-constructed or move-constructed temporary. When the operation is done, the temporary is deleted, and its destructor is called.

The swap behaviour is there to be able to reuse the destructor, so you don't have to write any cleanup code in your assignment operators.

If there's no cleanup behaviour to be done and only assignment, then you should be able to declare the assignment operators as default and copy-and-swap isn't needed.

The move constructor itself usually doesn't require any clean-up behaviour, since it's a new object. The general simple approach is to make the move constructor invoke the default constructor, and then swap all the members with the move-from object. The moved-from object will then be like a bland default-constructed object.

However, in this question's observer pattern example, that's actually an exception where you have to do extra cleanup work because references to the old object need to be changed. In general, I would recommend making your observers and observables, and other design constructs based around references, unmovable whenever possible.

Andeee

Answer 2

First of all, it is generally unnecessary to write a swap function in C++11 as long as your class is movable. The default swap will resort to moves:

void swap(T& left, T& right) {
    T tmp(std::move(left));
    left = std::move(right);
    right = std::move(tmp);
}

And that's it, the elements are swapped.

Second, based on this, the Copy-And-Swap actually still holds:

T& T::operator=(T const& left) {
    using std::swap;
    T tmp(left);
    swap(*this, tmp);
    return *this;
}

// Let's not forget the move-assignment operator to power down the swap.
T& T::operator=(T&&) = default;

Will either copy and swap (which is a move) or move and swap (which is a move), and should always achieve close to the optimum performance. There might be a couple redundant assignments, but hopefully your compiler will take care of it.

EDIT: this only implements the copy-assignment operator; a separate move-assignment operator is also required, though it can be defaulted, otherwise a stack overflow will occur (move-assignment and swap calling each other indefinitely).

Fatherinlaw

I agree that the cost of a swap is probably negligible if you're going to move anyway, but will it ever be advantageous over just reusing the code from the move constructor's body? As shown in my question, you can just put the move constructor's body in a seperate function to avoid code duplication. It seems to me that swapping is equal at best, and otherwise worse than moving. Essentially, what I miss in this answer is a reason why swap is a better choice. – Andeee

@Aberrant: I think it's mostly due to the age of the idiom. Copy and swap was obviously the way to go before we had rvalues. – Tlemcen

@Aberrant: Implementing the move constructor's body via a helper function is not particularly effective, because most of the work done by a constructor is in the initializer-list, not the body. – Adal

@Aberrant: swapping is indeed equal at best, in terms of performance, however it's also code you do not have to write and maintain; specifically, when you add a new data-member, you have much maintenance to do if you write the copy/move constructors and swap functions by hand, whereas the default versions generated by the compiler are always up-to-date. And as for a performance hit... well in fact there should be none. Moves are cheap (to begin with), and as long as the optimizer can inline the move constructor in the body of the swap specialization, it will remove the inefficiencies. – Fatherinlaw

Matthieu: Does using std::swap in the assignment operator really improve maintainability over a custom swap? A custom swap allows you to implement a trivial move constructor. Without it you need to implement move construction piecemeal, being careful to clear appropriate members from the source (or swap them with default-constructed members anyway)...which is harder than writing a member-wise swap. Also, wouldn't you recurse between std::swap and your assignment operator unless you wrote a separate member-wise move assignment operator (and watch for self-assignment from std::moved lvalues)? – Predicament

You generally can't use the default move constructor or move assignment operator either in this case: You usually only need to write a custom assignment operator if you have a non-trivial destructor...but if so, the default move constructor and move assignment operator won't properly free resources before copying over their handles with a simple copy, and the source won't be appropriately cleared either, so its destructor will free the destination's new resources too. – Predicament

@MikeS: I am afraid I do not understand everything here; so I'll answer what I groked. 1. There is no need to clear members in a move constructor, since by definition there is nothing yet, thus you just need to move each element piecemeal... which the compiler is generally happy to do for you = default. Therefore, implementing a move constructor is trivial. 2. There is no need to watch for self-assignment in a move assignment operator, a xr-value is a temporary (thus unnamed) and therefore cannot also appear on the left-hand side of an assignment... – Fatherinlaw

1. Right, I misspoke lumping the move constructor in with move assignment when I talked about freeing resources first. However, I'm still seeing another problem: The default move constructor moves member-wise from source to destination, preserving the destination. If both still reference the same resources (e.g. memory), and the source's destructor frees them when it's called, the destination will suddenly have dangling references. 2. Consider "a = std::move(a);" In real usage it would be more complicated, but wouldn't that bind an lvalue to an rvalue reference and self-assign? – Predicament

a = std::move(a) is undefined behavior (as per specs), you may choose to guard against it, but you do not have to. The containers in libc++ do not, for example. Regarding destination and source pointing to the same resource: this does not occur if all attributes have proper move constructors/assignment operators. Effectively a raw pointer does not behave correctly, but then the issue is using a raw pointer to point to an owned resource => you should have been using a unique_ptr instead. – Fatherinlaw

To elaborate on 2.: std::move(a) is considered an xvalue, but "a = std::move(a)" is still self-move-assignment. We're dealing with a non-POD, resource-owning class, which is why we're defining our own assignment operators to begin with. If the destination currently owns some resource, you can a.) Move assign with swap (copy-assign with copy-and-swap) or similar, or b.) Check for self-assignment, free current resources, then assign member-wise. If you do b. without the self-assignment check and do "a = std::move(a)," you'll free a's resources, then reassign dangling references. – Predicament

Sorry about above: I deleted and reposted to clarify at the wrong time. ;) It's news to me that the standard states "a = std::move(a)" is undefined; why does it not do the same for "a = a" though? Obviously both will be camouflaged through containers or references or similar in real code, but the principle seems the same to me, and we've been working around self-copy-assignment for years. – Predicament

As far as raw pointers and unique_ptr's go: We're talking about user-defined assignment operators here, so we're talking about something more like unique_ptr's implementation. Alternately, consider the implementation of any other class that directly owns resources. If your class only owns resources indirectly through RAII-managed members like vector, shared_ptr, etc. (i.e. it doesn't have to do any RAII management itself), you won't need user-defined destructors, constructors, or assignment operators at all, unless one of your class members is something odd like unique_ptr (non-copyable). – Predicament

@MikeS: Regarding move self-assignment, see here for an extended discussion (Howard Hinnant is the writer of libc++ and part of the Standard Library committee). Regarding implementation of unique_ptr: it's already implemented, so you don't have to ;) I would rather focus rule of thumbs of what a casual user has to write, rather than account for all the dirty stuff the maintainer of the Standard Library mess up with. – Fatherinlaw

Thanks for the link. I'm not talking about rewriting unique_ptr specifically btw. My point is that users will rarely need to write custom assignment operators at all, unless they're a.) using something weird like the non-copyable unique_ptr, or b.) working with a non-trivial destructor (i.e. directly managing some type of resource themselves). As a result of b., any discussion about how to write user-defined assignment operators has to accept as its premise the likelihood that these operators 1.) may have to free resources before assigning members, and 2.) may have to clear the source. – Predicament

Let's bring this back to your original answer: If the default move assignment operator is sufficient, why use copy-and-swap for the copy assignment operator? Why is default copy assignment broken? You only need to consider copy-and-swap if your assignment operator has to free the destination's existing raw resources (self-assignment suicide risk) or allocate new ones (throw risk). In the former case, default move assignment will be broken too: Even if self-move-assignment isn't a concern (debatable), you'll still have to free the same existing resources as you would with copy assignment. – Predicament

@MatthieuM. a = std::move(a) is only undefined for std::lib types, not necessarily for user-defined types. Secondly, doesn't the code above go into infinite recursion, where the generic swap does an assignment, which calls swap, which does an assignment, ... – Quillen

@JonathanWakely: Yes, there is a risk; I've tweaked the answer to better prevent it, but I cannot think of a signature of the copy-assignment operator which might not cause a stack-overflow with Copy-And-Swap if someone does not also implement the move-assignment operator (defaulted if possible). – Fatherinlaw

Well basically you want a custom swap to use copy-and-swap, so I disagree with this answer :) – Quillen

@JonathanWakely: I would say it depends if the move assignment can be auto-generated or not. If you use a unique_ptr as a member, then you get both move-constructor and move-assignment operators for free, and therefore you also get swap for free. However you need to write a custom copy-constructor and a custom copy-assignment operator (which can then use copy-and-swap). – Fatherinlaw

In that situation I would say the OP's copy-and-move is a better choice. – Quillen

@JonathanWakely: better in which terms ? My primary purpose is to minimize the number of times I have to enumerate the set of fields, because the more times I have to enumerate them, the more susceptible I am to miss one in some method during a later maintenance operation. With the method I propose, you only enumerate the fields in the "regular" constructors you write and (if necessary) in the copy constructor; and everything else is taken care of automatically. The OP's solution is thus worse in this regard, for no performance gain that I can see. – Fatherinlaw

In the copy assignment, why do you capture by reference T& T::operator=(T const& left) instead of by value T& T::operator=(T left)? Wouldn't capturing by value obviate the need for tmp? – Necessitous

@becko: Not only it would, it would also obviate the need of writing two assignment operators. – Fatherinlaw

Answer 3

Give each special member the tender loving care it deserves, and try to default them as much as possible:

class MyClass
{
private:
    BigClass data;
    std::unique_ptr<UnmovableClass> dataPtr;

public:
    MyClass() = default;
    ~MyClass() = default;
    MyClass(const MyClass& other)
        : data(other.data)
        , dataPtr(other.dataPtr ? new UnmovableClass(*other.dataPtr)
                                : nullptr)
        { }
    MyClass& operator=(const MyClass& other)
    {
        if (this != &other)
        {
            data = other.data;
            dataPtr.reset(other.dataPtr ? new UnmovableClass(*other.dataPtr)
                                        : nullptr);
        }
        return *this;
    }
    MyClass(MyClass&&) = default;
    MyClass& operator=(MyClass&&) = default;

    friend void swap(MyClass& first, MyClass& second)
    {
        using std::swap;
        swap(first.data, second.data);
        swap(first.dataPtr, second.dataPtr);
    }
};

The destructor could be implicitly defaulted above if desired. Everything else needs to be explicitly defined or defaulted for this example.

Reference: http://accu.org/content/conf2014/Howard_Hinnant_Accu_2014.pdf

The copy/swap idiom will likely cost you performance (see the slides). For example ever wonder why high performance / often used std::types like std::vector and std::string don't use copy/swap? Poor performance is the reason. If BigClass contains any std::vectors or std::strings (which seems likely), your best bet is to call their special members from your special members. The above is how to do that.

If you need strong exception safety on the assignment, see the slides for how to offer that in addition to performance (search for "strong_assign").

Annapolis

re "see the slides", it's generally better to include the relevant information in the SO answer than to direct the user off-site. and at least in my firefox the slides do not provide information about the performance. see this screenshot. btw. i'm just guessing where in the slides the info probably is, if it is there. – Quadrilateral

Why the explicit swap here, instead of the default? – Tlemcen

The slides don't explain why copy and swap is slower, they just say that it is. Also I don't understand why the slides mention capacity since swapping a vector is just swapping pointers with no changes in capacity. – Macintyre

Would you consider separating the strong exception guarantee from the operator= implementation an application of the Single Responsibility Principle? – Adal

@MooingDuck: re: swap. I was mainly just copy/pasting from the original question. It might have some performance benefits too. – Annapolis

@DavidBrown: The key is recycling capacity when it is sufficient to do the job. In the extreme case, (the right hand side of the graph), capacity of the lhs is always sufficient to handle the size of the rhs, and thus one never needs to deallocate/allocate memory. Avoiding trips to the heap is key to optimization. – Annapolis

@BenVoigt: I think of it more along the lines of the venerable: don't pay for what you don't use. Sure, strong exception safety can be very handy. But it can also be expensive. I only want to pay for it when I actually need it. – Annapolis

@Cheersandhth.-Alf: It is a pdf. Perhaps if you downloaded it and tried viewing it with something besides Firefox (Acrobat?) you would have better luck. – Annapolis

@HowardHinnant: downloaded pdf worked nicely in Sumatra, but, the viewer in Firefox (I don't know what it is, possibly Mozilla's own) has not screwed up before, so there's something probably non-standard about this pdf. – Quadrilateral

@Cheersandhth.-Alf: Thanks for the report, and sorry for your experience. There is really nothing I can do to alter that pdf at this point. It was exported from Apple's Keynote presentation application. – Annapolis

There is much existing practice (e.g. reportedly in google) of just ignoring the possibility of memory exhaustion. And then in many cases copying can't fail. And when it can't fail, then there is no reason to choose a safe but needlessly slow copying implementation. But we're then into the correctness versus speed debate: does it matter whether software is correct, if it's fast enough? E.g. can classes later be updated so copying fails for other reasons than memory? And also, can we differentiate terminating from ordinary exceptions? I think this area is still unclear. – Quadrilateral

@Alf: Add to that the fact that on Linux default configuration, you can't handle memory exhaustion, and it's easy to see why people don't plan for behavior when it happens. But I think we're actually talking about address-space exhaustion anyway. – Adal

I am sympathetic to the "my system won't throw, it will instead thrash itself to death" position. However a custom allocator might be installed way down in the data structure somewhere that says something along the lines of: if I've allocated over a 1Gb, just give up and throw bad_alloc. I.e. new and malloc aren't the only allocators in town. And the more complicated your data structure, or the more generic your data structure, the more likely it is that a custom allocator could come into play. – Annapolis

Would replacing the raw new with make_unique be considered an improvement? – Heft

@TemplateRex: In this example it would neither help nor hurt. One of the reasons I did not use it above is that make_unique doesn't become standard until C++14 (which actually isn't official yet, but C++14 is a pretty safe bet right now). I wanted to keep my answer C++11. In general, once you have make_unique in your toolbox, I would encourage its use so that one does not have to decide whether or not it is an improvement. make_unique is never worse, and sometimes better. – Annapolis

I don't quite understand why you check for self-assignment. Is it intended to be an optimization? – Hockenberry

@dyp: Good question. To tell you the truth it was the unique_ptr::reset that had me nervous. On reflection, that would probably be ok under self-copy-assignment. But I didn't invest the time to make sure. – Annapolis

By request, I've edited the question to make it less verbose. The code that your answer focuses on is now mostly removed, since the code involving the observable pattern did a better job at demonstrating the issue (since the registration and unregistration could not be defaulted). I think your answer regrettably lost some relevance in the process, so you may want to review it. – Andeee

@Cheersandhth.-Alf the integrated Javascript PDF plugin in Firefox is just plain terrible: go to about:config and set pdfjs.disabled to true. Then your sumatrapdf or Adobe plugin should kick in like before. – Animosity