6

Is it a good general practice to always implement my copy assignment operators using std::swap? My understanding is that this provides a way to share the copy constructor implementation. I'd like to avoid duplicating the actual copy logic itself. So here is what I'd do:

class Foo
{
public:
    Foo(Foo const& other) { /* assume valid implementation */ }

    Foo& operator= (Foo other)
    {
        std::swap(*this, other);
        return *this;
    }
};

The act of passing "other" into the assignment operator performs copy construction (we've shared the copy logic at this point). I assume the swap will invoke move construction (which has a compiler-generated implementation here).

I've been doing this to pretty much every class I implement copy construction for, since the assignment & constructor never have different implementations.

void.pointer
  • 24,859
  • 31
  • 132
  • 243
  • Yes, copy and swap idiom is a good one for any resource owning class. Another benefit is the strong exception guarantee that you get from the swap. Being a "general rule" for a code base if more opinion based, imo. – quantdev Sep 19 '14 at 20:55
  • 2
    You should be `using namespace std;` before `swap` and remove the `std::`. That way a more specific `swap` is used if available. Other than that this is pretty standard. – nwp Sep 19 '14 at 20:56
  • 2
    Some people argue that implementing special member functions in terms of `swap` is not optimal. There are some unnecessary assignments since `swap` brings `other` into some specific state. Also, assignment operators providing the strong exception guarantee might be a pessimization: The user might not need that guarantee, but they'll pay for it in any case. – dyp Sep 19 '14 at 21:00
  • @dyp Good point but I'd argue from a design perspective that going back and making things exception safe later is a big pain and sometimes it's not something you can do without discussing complete rewrites. Like localization and many other foundational things, it's something best to do up front. – void.pointer Sep 19 '14 at 21:09
  • "(which has a compiler-generated implementation here)." -- you blocked your compiler-generated implementation in your sample code. If you intend to have it, add the proper `=default` – Yakk - Adam Nevraumont Sep 19 '14 at 21:11
  • @Yakk strangely I do not define any move construction/assignment in my classes and the `swap()` call does not fail to compile. Why would it still work? – void.pointer Sep 19 '14 at 21:12
  • Copy constructor is a valid fall back move constructor. Your `operator=` is a valid move assign. – Yakk - Adam Nevraumont Sep 19 '14 at 21:14
  • @Yakk where are the rules for this defined? I assume there is some sort of precedence... i.e. it will try true move construction first, if that isn't there, it will try lvalue reference, then lvalue? – void.pointer Sep 19 '14 at 21:22
  • @void.pointer This isn't about "making things exception safe", it's about *which* exception guarantee you want to provide for you assignment-operator: The strong one (if something goes wrong, there are no effects) or the basic one (no resources are leaked). – dyp Sep 19 '14 at 21:22
  • @void.pointer whuch it calls is just overloading: `T` and `T const&` are valid for an arg of type `T&&`. Which is generated... should be a stack overflow answer somewhere around here. – Yakk - Adam Nevraumont Sep 20 '14 at 02:48

2 Answers2

10

Your code does not have a move constructor. Your copy constructor blocks the automatic creation of a move constructor, and attempting to move your class instead copies it.

For move-assign, your operator= also blocks its automatic implementation, and can be used in its place.

The end result is an infinite recursive call of = (live code).

If you follow the rule of 0, you need neither a copy constructor, move constructor, move-assignment, copy-assignment, or destructor. If you write any of them, you should be prepared to write all of them.

Using std::swap can be useful, but because you have to write your move-assign and move-construct, doing either in terms of std::swap is an infinite recursion waiting to happen.

Yakk - Adam Nevraumont
  • 262,606
  • 27
  • 330
  • 524
  • 3
    o.O You're right: You cannot implement `operator=` in terms of `std::swap`; you can implement it in terms of your *own* ("optimized") `swap`. The usual copy-and-swap idiom for operators uses a custom `swap` (member function). – dyp Sep 19 '14 at 21:26
  • I think if I just add the move constructor, that fixes the infinite recursion (good catch btw). Move assignment will still be a copy, but the swap will end up using the move constructor, right? – void.pointer Sep 19 '14 at 21:32
  • @void.pointer `std::swap` does this: `swap(A& a, A& b) { A tmp( move(a) ); a = move(b); b = move(tmp); }` So it needs a move assignment-operator. – dyp Sep 19 '14 at 21:37
  • 1
    @dyp: Technically, `std::swap` needs `T` to be *move assignable*. `T` does not need a move assignment operator to be move assignable. For example a copy assignment operator can fulfill the move assignable requirement. – Howard Hinnant Sep 19 '14 at 21:43
  • @HowardHinnant Hmm, that was intended as a simplification, but it oversimplified. It should say "So it calls an assignment-operator." (which then leads to infinite recursion in the OP's code) -- this is still a simplification, since e.g., for trivially copyable types it's not required to call an operator. – dyp Sep 19 '14 at 21:47
  • 1
    @dyp: I've read enough of your posts to know that you know this. It is just that when slightly incorrect things get repeated, newbies tend to believe them to be completely correct. E.g. the copy swap idiom is the best way to implement assignment... ;-) – Howard Hinnant Sep 19 '14 at 21:56
9

If Foo contains non-static data members std::vector or std::string, or contains data members that contain vector or string (i.e. even indirectly), then this can be a very effective way to slow your code down. It can even be more effective than calling std::sleep_for as the latter doesn't waste battery power on mobile devices.

The reason is that a copy assignment which calls the vector or string copy assignment has the chance to reuse the container's capacity. Whereas the swap idiom always throws away capacity.

For more info, see my ACCU 2014 talk, slides 43-53.

In particular, note this performance chart which shows the speed increase of calling vector's copy assignment operator vs doing the copy/swap idiom with a vector data member.

this http://howardhinnant.github.io/accu_2014_48.pdf

At best, the copy/swap idiom is as fast as using vector's copy assignment (when capacity is never sufficient). At worst (when capacity is always sufficient), copy/swap is nearly 8X slower. On average, copy/swap entails a 70% speed hit (for this test).

Howard Hinnant
  • 206,506
  • 52
  • 449
  • 577
  • 1
    I think this comes down to whether or not you want/need to provide the strong exception guarantee. But IIRC, you're in favor of external control à la `strong_assign`. – dyp Sep 19 '14 at 21:21
  • Just linking to this question with further discussion https://stackoverflow.com/questions/51463356/copy-swap-idiom-not-recommended – alfC Sep 18 '18 at 09:53