c++ - Why do some people use swap for move assignments?

Question

For example, stdlibc++ has the following:

unique_lock& operator=(unique_lock&& __u)
{
    if(_M_owns)
        unlock();
    unique_lock(std::move(__u)).swap(*this);
    __u._M_device = 0;
    __u._M_owns = false;
    return *this;
}

Why not just assign the two __u members to *this directly? Doesn't the swap imply that __u is assigned the *this members, only to later have then assigned 0 and false... in which case the swap is doing unnecessary work. What am I missing? (the unique_lock::swap just does an std::swap on each member)

Answer 1

It's my fault. (half-kidding, half-not).

When I first showed example implementations of move assignment operators, I just used swap. Then some smart guy (I can't remember who) pointed out to me that the side effects of destructing the lhs prior to the assignment might be important (such as the unlock() in your example). So I stopped using swap for move assignment. But the history of using swap is still there and lingers on.

There's no reason to use swap in this example. It is less efficient than what you suggest. Indeed, in libc++, I do exactly what you suggest:

unique_lock& operator=(unique_lock&& __u)
    {
        if (__owns_)
            __m_->unlock();
        __m_ = __u.__m_;
        __owns_ = __u.__owns_;
        __u.__m_ = nullptr;
        __u.__owns_ = false;
        return *this;
    }

In general a move assignment operator should:

1. Destroy visible resources (though maybe save implementation detail resources).
2. Move assign all bases and members.
3. If the move assignment of bases and members didn't make the rhs resource-less, then make it so.

Like so:

unique_lock& operator=(unique_lock&& __u)
    {
        // 1. Destroy visible resources
        if (__owns_)
            __m_->unlock();
        // 2. Move assign all bases and members.
        __m_ = __u.__m_;
        __owns_ = __u.__owns_;
        // 3. If the move assignment of bases and members didn't,
        //           make the rhs resource-less, then make it so.
        __u.__m_ = nullptr;
        __u.__owns_ = false;
        return *this;
    }

Update

In comments there's a followup question about how to handle move constructors. I started to answer there (in comments), but formatting and length constraints make it difficult to create a clear response. Thus I'm putting my response here.

The question is: What's the best pattern for creating a move constructor? Delegate to the default constructor and then swap? This has the advantage of reducing code duplication.

My response is: I think the most important take-away is that programmers should be leery of following patterns without thought. There may be some classes where implementing a move constructor as default+swap is exactly the right answer. The class may be big and complicated. The A(A&&) = default; may do the wrong thing. I think it is important to consider all of your choices for each class.

Let's take a look at the OP's example in detail: std::unique_lock(unique_lock&&).

Observations:

A. This class is fairly simple. It has two data members:

mutex_type* __m_;
bool __owns_;

B. This class is in a general purpose library, to be used by an unknown number of clients. In such a situation, performance concerns are a high priority. We don't know if our clients are going to be using this class in performance critical code or not. So we have to assume they are.

C. The move constructor for this class is going to consist of a small number of loads and stores, no matter what. So a good way to look at the performance is to count loads and stores. For example if you do something with 4 stores, and somebody else does the same thing with only 2 stores, both of your implementations are very fast. But their's is twice as fast as yours! That difference could be critical in some client's tight loop.

First lets count loads and stores in the default constructor, and in the member swap function:

// 2 stores
unique_lock()
    : __m_(nullptr),
      __owns_(false)
{
}

// 4 stores, 4 loads
void swap(unique_lock& __u)
{
    std::swap(__m_, __u.__m_);
    std::swap(__owns_, __u.__owns_);
}

Now lets implement the move constructor two ways:

// 4 stores, 2 loads
unique_lock(unique_lock&& __u)
    : __m_(__u.__m_),
      __owns_(__u.__owns_)
{
    __u.__m_ = nullptr;
    __u.__owns_ = false;
}

// 6 stores, 4 loads
unique_lock(unique_lock&& __u)
    : unique_lock()
{
    swap(__u);
}

The first way looks much more complicated than the second. And the source code is larger, and somewhat duplicating code we might have already written elsewhere (say in the move assignment operator). That means there's more chances for bugs.

The second way is simpler and reuses code we've already written. Thus less chance of bugs.

The first way is faster. If the cost of loads and stores is approximately the same, perhaps 66% faster!

This is a classic engineering tradeoff. There is no free lunch. And engineers are never relieved of the burden of having to make decisions about tradeoffs. The minute one does, planes start falling out of the air and nuclear plants start melting down.

For libc++, I chose the faster solution. My rationale is that for this class, I better get it right no matter what; the class is simple enough that my chances of getting it right are high; and my clients are going to value performance. I might well come to another conclusion for a different class in a different context.