9

This is related to this answer provided by Matthieu M. on how to utilize move semantics with the + operator overloading (in general, operators which don't re-assign directly back to the left param).

He suggested implementing three distinct overloads:

inline T operator+(T left, T const& right) { left += right; return left; }
inline T operator+(T const& left, T right) { right += left; return right; } // commutative
inline T operator+(T left, T&& right) { left += right; return left; } // disambiguation

Number 1 and 3 make sense, but I don't understand what purpose 2 does. The comment suggests commutative handling, but it seems that 1 and 2 would be mutually exclusive (i.e. implementing both results in ambiguities)

For example, with all 3 implemented:

T a, b, c;
c = a + b;

Compiler output:

1>          error C2593: 'operator +' is ambiguous
1>          could be 'T operator +(const T &,T)'
1>          or       'T operator +(T,const T &)'
1>          while trying to match the argument list '(T, T)'

removing either 1 or 2 and the program works as expected. Since 1 is the general case and 2 only works correctly with a commutative operator, I don't see why 2 would ever be used. Is there something I'm missing?

Community
  • 1
  • 1
helloworld922
  • 10,801
  • 5
  • 48
  • 85

2 Answers2

12

I don't think you're missing anything -- the code in your question is indeed trouble. The earlier part of his answer made sense, but something was lost between the "four desired cases" and the actual example.

This could be better:

inline T operator+(T left, T const& right) { left += right; return left; }
inline T operator+(const T& left, T&& right) { right += left; return right; }

This implements the rule: Make a copy of the LHS (preferably via move construction), unless the RHS is expiring anyway, in which case modify it in place.

For non-commutative operators, omit the second overload, or else provide an implementation that doesn't delegate to compound assignment.

If your class has heavyweight resources embedded inside (so that it can't be efficiently moved), you'll want to avoid pass-by-value. Daniel makes some good points in his answer. But do NOT return T&& as he suggests, since that is a dangling reference.

Ben Voigt
  • 277,958
  • 43
  • 419
  • 720
  • Am I correct in saying that if op(left,right) is not commutative correct handling can be done by just ignoring the second overload (const T& left, T&& right)? – helloworld922 Apr 21 '13 at 22:11
  • @helloworld922: Yes, that's correct. In such a case the right-hand-side cannot be modified in-place, so there's no speed advantage to the second overload. – Ben Voigt Apr 21 '13 at 22:57
  • Which one gets called if both are rvalues? Can we add another one `inline T operator+(T&&,T&&)` and choose which one to reuse based on which one has bigger resource? – balki Apr 22 '13 at 16:48
  • @balki: In my case, the second overload gets called. Yes your suggestion will work -- whether it is worthwhile depends on the specifics of your data type. – Ben Voigt Apr 22 '13 at 21:36
  • Shouldn't that be "inline T operator+(const T& left, T&& right) { right += left; return std::move(right); }"? Because inside the function right is a lvalue, so the return will make a copy of right. – Dominic Hofer Mar 27 '16 at 16:05
6

Important update / warning about this answer!

There actually is a convincing example which silently creates a dangling reference in reasonable real-world code with the below. Please use the other answer's technique to avoid this problem even at the cost of some additional temporaries being created. I'll leave the rest of this answer untouched for future reference.


The correct overloads for a commutative case are:

T   operator+( const T& lhs, const T& rhs )
{
  T nrv( lhs );
  nrv += rhs;
  return nrv;
}

T&& operator+( T&& lhs, const T& rhs )
{
  lhs += rhs;
  return std::move( lhs );
}

T&& operator+( const T& lhs, T&& rhs )
{
  rhs += lhs;
  return std::move( rhs );
}

T&& operator+( T&& lhs, T&& rhs )
{
  lhs += std::move( rhs );
  return std::move( lhs );
}

Why is that and how does it work? First, notice that if you take an rvalue reference as a parameter, you can modify and return it. The expression where it comes from needs to guarantee that the rvalue won't be destructed before the end of the complete expression, including operator+. This also means that operator+ can simply return the rvalue reference as the caller needs to use the result of operator+ (which is part of the same expression) before the expression is completely evaluated and the temporaries (ravlues) are destructed.

The second important observation is, how this saves even more temporaries and move operations. Consider the following expression:

T a, b, c, d; // initialized somehow...

T r = a + b + c + d;

with the above, it is equivalent to:

T t( a );    // T operator+( const T& lhs, const T& rhs );
t += b;      // ...part of the above...
t += c;      // T&& operator+( T&& lhs, const T& rhs );
t += d;      // T&& operator+( T&& lhs, const T& rhs );
T r( std::move( t ) ); // T&& was returned from the last operator+

compare this to what happens with the other approach:

T t1( a );   // T operator+( T lhs, const T& rhs );
t1 += b;     // ...part of the above...
T t2( std::move( t1 ) ); // t1 is an rvalue, so it is moved
t2 += c;
T t3( std::move( t2 ) );
t3 += d;
T r( std::move( t3 );

which means you still have three temporaries and although they are moved instead of copied, the approach above is much more efficient in avoiding temporaries altogether.

For a complete library, including support for noexcept, see df.operators. There you will also find versions for non-commutative cases and operations on mixed types.


Here's a complete test program to test it:

#include <iostream>
#include <utility>

struct A
{
  A() { std::cout << "A::A()" << std::endl; }
  A( const A& ) { std::cout << "A::A(const A&)" << std::endl; }
  A( A&& ) { std::cout << "A::A(A&&)" << std::endl; }
  ~A() { std::cout << "A::~A()" << std::endl; }

  A& operator+=( const A& ) { std::cout << "+=" << std::endl; return *this; }
};

// #define BY_VALUE
#ifdef BY_VALUE
A operator+( A lhs, const A& rhs )
{
  lhs += rhs;
  return lhs;
}
#else
A operator+( const A& lhs, const A& rhs )
{
  A nrv( lhs );
  nrv += rhs;
  return nrv;
}

A&& operator+( A&& lhs, const A& rhs )
{
  lhs += rhs;
  return std::move( lhs );
}
#endif

int main()
{
  A a, b, c, d;
  A r = a + b + c + d;
}
Community
  • 1
  • 1
Daniel Frey
  • 55,810
  • 13
  • 122
  • 180
  • You really don't need that many versions. See http://cpp-next.com/archive/2009/08/want-speed-pass-by-value/ The first version in my answer looks like it may involve an extra move operation, but this can and will be elided by the compiler during inlining. – Ben Voigt Apr 21 '13 at 22:56
  • 1
    @BenVoigt No, it doesn't. **Try it out!** (I'm so sick of hearing of this article and everyone thinking that pass-by-value solves everything. It doesn't. Again: Try it out.) – Daniel Frey Apr 21 '13 at 23:05
  • I can see why having all 4 overloads would be considered more complete and efficient by avoiding a few temporaries, but I think the statement that it's **much** more efficient in many cases is an over-statement since most of the time I have large heap data to move and little other data which would need to be initialized individually for each temporary. Of course, like any optimization it should be benchmarked to assess any true performance gains. – helloworld922 Apr 21 '13 at 23:23
  • 1
    @helloworld922 Please don't quote me like that. I said "much more efficient in avoiding temporaries", not "much more efficient in many cases". In some cases you might not see a difference because of other optimizations later on and/or because your use-case simply can't benefit from it, but the important part is that my version is *never* less efficient, so it *never* hurts. Also, using a library means you don't even need to care. Concentrate on getting `operator+=` correct and leave the rest to [df.operators](https://github.com/d-frey/operators/). – Daniel Frey Apr 21 '13 at 23:29
  • @DanielFrey Ah yes, missed that quantifier at the end. At the end of the day it's a choice between "doing it right", or taking shortcuts which will still work, but isn't completely optimal. Either way, two distinct good solutions for different needs. – helloworld922 Apr 21 '13 at 23:34
  • @DanielFrey: Considering that article comes from members of the standard committee, one is led to blame the incomplete implementation of C++11 in whatever compiler you tested, and expect that optimizers will start handling this case quite soon. – Ben Voigt Apr 22 '13 at 06:28
  • 1
    @BenVoigt Have you read the "Reality Bites" section of Dave's article? Even if a compiler will be able to do that kind of optimization in the future (especially for non-trivial cases where optimizations really matter), it means a lot of pressure on the optimization pass. My technique works today (tested on GCC and Clang). – Daniel Frey Apr 22 '13 at 06:58
  • @Daniel: Even when the call to the move constructor isn't eliminated, it will be inlined, and after optimization there ought to be nothing left. Writing to `cout` from a constructor is a very unusual case. – Ben Voigt Apr 22 '13 at 21:37
  • @BenVoigt `cout` shows the side-effects and therefore also those cases where things become interesting. If your class only contains one integer/float or a pointer to some data it owns, the compiler might optimize it and you won't see a difference, but what about a small matrix class with 4x4 = 16 floats that are *not* separately allocated? Moving makes almost no sense and you end up with copies. Those are the cases where additional temporaries really hurt. Also, consider a class where the internal pointer to the data is thread-safe: Every move leads to memory-barriers. – Daniel Frey Apr 23 '13 at 06:34
  • @DanielFrey: Moves shouldn't lead to memory barriers. If your object is accessible from other threads, it is not a candidate for moving. You don't need to lock an object under construction (unless you allow the `this` pointer to leak), and you don't need to lock an object being moved from. In both cases you have exclusive access to the object already, and don't need a mutex to ensure this. – Ben Voigt Apr 23 '13 at 15:27
  • @BenVoigt You are still concentrating on the cases where additional temporaries don't matter, assuming "sufficiently smart compilers" or specific details about implementations. Fine. I still think there are also cases like the matrix class you so conveniently ignored where the difference *does* matter. – Daniel Frey Apr 23 '13 at 16:24
  • @DanielFrey: Yes, for non-movable classes it makes sense to handle rvalue references specially. I don't think non-movable classes were the topic of this question, tho. – Ben Voigt Apr 23 '13 at 16:48
  • @BenVoigt I think the question was about using move-semantics with `operator+`. My code does that to reduce temporaries, even if the class *itself* has no move-ctor! Try commenting out the move-ctor in the test program I gave in the answer. :) – Daniel Frey Apr 23 '13 at 17:09
  • Ok, I can see that. Added some text to my answer explaining when the additional overloads from your answer would be useful. and +1 – Ben Voigt Apr 23 '13 at 17:48
  • Uh-oh... actually this is broken, since you return a dangling reference. http://ideone.com/n7vJl1 – Ben Voigt Apr 23 '13 at 17:55
  • @BenVoigt Thanks for the +1, but for the "this is broken": You have only broken your own assumptions that I never guaranteed. Since SO is not for discussion, may I invite you (and everyone else interested) to the Boost devel mailing list where I'm also discussing the future of Boost.Operators right now? Thanks! – Daniel Frey Apr 23 '13 at 18:25
  • @DanielFrey: What is broken is your assumption (claimed in your answer) that a temporary object cannot survive beyond the end of the containing *full-expression*. – Ben Voigt Apr 23 '13 at 19:45
  • @BenVoigt But I don't guarantee that I return a temporary object. Hence, you can not assume that you can extend the lifetime of something you don't get from the operator. – Daniel Frey Apr 23 '13 at 20:15
  • 1
    @DanielFrey: An operator overload that can't be used in the same manner as *every* built-in type and *every* Standard library type is broken. – Ben Voigt Apr 23 '13 at 20:29