How to implement copy constructor for copy swap idiom

Question

I am trying to implement a copy constructor and a assignment operator for a class. I am getting a bit confused with the copy swap idiom. Especially when it comes to the copy constructor. Does the copy swap idiom have anything to do with a copy constructor at all? How do I avoid code duplication?

Here is my class

Header:

Class Actor
{
public:
    Foo* foo;
    Bar bar;
    double member1;
    bool member2;
    unsigned int member3;

    void Swap(Actor& first, Actor& second);
    Actor(const Actor&);
    Actor& operator=(const Actor);
}

Cpp:

void Actor::Swap(Actor& first, Actor& second)
{
    // Swap wont work with my non pointer class
    Bar temp = first.bar;
    first.bar = second.bar;
    second.bar = temp;

    std::swap(first.foo, second.foo);
    std::swap(first.member2, second.member2);
    std::swap(first.member3, second.member3);
}

// What goes here? Is this a correct copy constructor? Does this have anything to do with a copy swap idiom? How can I avoid code duplication in my copy constructor?
Actor::Actor(const Actor& other)
{
    foo = new Foo();
    *foo = *other.foo;

    bar = other.bar;
    member1 = other.member1;
    member2 = other.member2;
    member3 = other.member3;
}

Actor& Actor::operator=(Actor other)
{
    Swap(*this, other);
    return *this;
}

I am following this guide: What is the copy-and-swap idiom?

Answer 1

How do I avoid code duplication?

Truthfully, the main motivation behind copy-and-swap (CAS) is less about avoiding code duplication, and more about giving the strong exception guarantee. This means that if you try to perform a copy assignment, and it fails, that the object you tried to assign to is not modified in any way. Generally speaking, move construction/assignment do not throw, and copy construction gives this guarantee implicitly, because if an exception is thrown than the object will not exist. So in the typical case, copy assignment is the "odd man out" that does not offer strong exception guarantee or better.

That said, strong exception guarantee is often hard to provide, and may not be that useful in practice. I would not encourage people to automatically assume that CAS is the best thing.

If your goal is to avoid code duplication, the best route is to use the Rule of Zero: http://en.cppreference.com/w/cpp/language/rule_of_three. Basically the idea is to choose individual members for your class, so that the compiler generated copy/move constructor/assignment is the behavior you want. I understand that this code is perhaps an exercise to gain understanding of these functions, but it's good to be aware of this when you are writing code for its own sake instead for learning.

Edit: A final note. Assuming you are using C++11, generally speaking the recommended way to use CAS would actually not be to write swap yourself. Instead, you would write the move constructor (which you have to anyway), and move assignment. From these two functions, the generic std::swap will be able to efficiently swap your class. And then you can use the generic swap in your CAS implementation. In some cases you might write swap yourself but it's usually not necessary. More detailed discussion here: http://scottmeyers.blogspot.com/2014/06/the-drawbacks-of-implementing-move.html.

Answer 2

The copy-swap approach is a way to implement a copy-assignment operator in terms of reusing your swap and copy constructor to reduce repeated code and add exception safety.

That said I noticed some thing about your code:

• You don't have a destructor so you will be leaking your Foo* foo; objects when your Actors are destructed.
• In your copy constructor you can allocate and copy the Foo in one step, without needing the intermediate default constructed Foo that you assign to.
• Your Swap function is a member that takes two arguments, which effectively means it's getting three arguments. It should be a single parameter member and swap with this or a two parameter non-member.
• For consistency with the standard library I recommend calling your swap function swap rather than Swap, regardless of which implementation approach you take.
• In your swap, prefer to delete the Bar swapping to Bar rather than implementing it in the Actor class: In other words let Bar know how to swap itself and simply leverage that functionality.

Answer 3

The copy-and-swap assignment idiom requires two mechanisms: A copy c'tor, and a non-throwing swap function.
So your code follows it to the letter.

The idea is to try and do the unsafe operation first, you create a copy simply by defining a new object. If it fails and throws, the object you are assigning to remains unchanged, it's state is predictable in case of an error. That's the exception safety guarantee.

If no error occurs on copy, you do a swap to finalize the assignment. Since the swap method is non-throwing, you are in no risk of leaving your assigned-to object in an inconsistent state.

Finally, the destructor of the temporary clears any and all left behind resources, since the swap method handed those to it.

Answer 4

I suppose that this implementation is well except a few things:

1) You forgot to swap member1 in your custom Swap method.

2) Copy-and-swap idiom usually provides a strong exception guarantee for operator=. It means that it either fails with an exception without changing the object or successfully perform assignment. In your case strong exception safety is questionable because Bar assignment can potentially throw an exception.