While I see no inherent concept flaw in your implementation, I would make three suggestions.
(note: I'll refer here to an implementation of the concept of swaping rvalues as swap_rvalues
)
Exclude const types from the template deduction
Assuming
template <typename A, typename B>
struct is_same_no_ref
: std::is_same<
typename std::remove_reference<A>::type,
typename std::remove_reference<B>::type
>
{};
change the enable condition from std::enable_if<std::is_same_no_ref<A, B>::value>
into the following.
std::enable_if<
std::is_same_no_ref<A, B>::value &&
!std::is_const<typename std::remove_reference<A>::type>::value &&
!std::is_const<typename std::remove_reference<B>::type>::value
>
Without excluding const types from the template deduction, passing const variables to swap_rvalues
, as in the following,
int const a = 0, b = 0;
swap_rvalues(a, b);
induces the compiler to flag errors regarding internal implementation details, what is not very user-friendly.
Move the enable condition to the return type declaration
Instead of
template<typename A, typename B, typename = typename std::enable_if<...>::type>
inline void swap_rvalues(A&& a, B&& b);
declare it like the following
template<typename A, typename B>
inline typename std::enable_if<...>::type swap_rvalues(A&& a, B&& b);
Even though highly improbable, the explicit definition of the third template parameter of swap_rvalues
is possible, effectively overriding the enable condition. This may allow code to compile which shouldn't and nastiness could follow. This is completely avoided using the return type declaration for the enable condition.
Consider the following example.
template <
typename A,
typename B,
typename = typename std::enable_if<
is_same_no_ref<A, B>::value &&
!std::is_const<typename std::remove_reference<A>::type>::value &&
!std::is_const<typename std::remove_reference<B>::type>::value
>::type>
inline void
swap(A&& a, B&& b) {
typename std::remove_reference<A>::type t = std::move(a);
a = std::move(b);
b = std::move(t);
}
struct B;
struct A{A(){} A(B const&){}};
struct B{B(){} B(A const&){}};
swap<A, B, void>(A(), B());
It compiles, even though it clearly shouldn't! A
and B
are not even related, they just happen to be constructable given a reference of the other.
Reuse code [aka KISS]
Since the rvalues are already given a name, simply forward call std::swap
, instead of providing a brand new implementation of swap_rvalues
.
Why reinventing the wheel†? std::swap
already provides the intended behavior once rvalues are given a name, so why not reusing it?
Conclusion
The final implementation of swap_rvalues
‡ would look like follows.
template <typename A, typename B>
inline typename std::enable_if<
is_same_no_ref<A, B>::value &&
!std::is_const<typename std::remove_reference<A>::type>::value &&
!std::is_const<typename std::remove_reference<B>::type>::value
>::type
swap_rvalues(A&& a, B&& b) {
std::swap(a, b);
}
Footnotes
† "To reinvent the wheel is to duplicate a basic method that has already previously been created or optimized by others."
‡ swap_rvalues
in fact would better be called swap
in a real scenario.
Step 1
Set up a performance test which exercises the move assignment operator.
Set up another performance test which exercises the copy assignment operator.
Step 2
Set up the assignment operator both ways as instructed in the problem statement.
Step 3
Iterate on Steps 1 and 2 until you have confidence that you did them correctly.
Step 3 should help educate you as to what is going on, most likely by telling you where the performance is changing and where it is not changing.
Guessing is not an option for Steps 1-3. You actually have to do them. Otherwise you will (rightly) have no confidence that your guesses are correct.
Step 4
Now you can start guessing. Some people will call this "forming a hypothesis." Fancy way of saying "guessing." But at least now it is educated guessing.
I ran through this exercise while answering this question and noted no significant performance difference on one test, and a 6X performance difference on the other. This further led me to an hypothesis. After you do this work, if you are unsure of your hypothesis, update your question with your code, results, and subsequent questions.
Clarification
There are two special member assignment operators which typically have the signatures:
HasPtr& operator=(const HasPtr& rhs); // copy assignment operator
HasPtr& operator=(HasPtr&& rhs); // move assignment operator
It is possible to implement both move assignment and copy assignment with a single assignment operator with what is called the copy/swap idiom:
HasPtr& operator=(HasPtr rhs);
This single assignment operator can not be overloaded with the first set.
Is it better to implement two assignment operators (copy and move), or just one, using the copy/swap idiom? This is what Exercise 13.53 is asking. To answer, you must try both ways, and measure both copy assignment and move assignment. And smart, well meaning people get this wrong by guessing, instead of testing/measuring. You have picked a good exercise to study.
Best Answer
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:
In general a move assignment operator should:
Like so:
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:
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:
Now lets implement the move constructor two ways:
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.