Re: [boost] [operators] A modern SFINAE-based version of boost::operators?

On 14. Nov 2017, at 09:43, Hans Dembinski wrote:

Dear Daniel,

...

On 13. Nov 2017, at 17:26, Daniel Frey via Boost wrote:

I worked on other improvements, which ultimately went into its own library: https://github.com/taocpp/operators. There, I distinguish between commutative and non-commutative versions of some operators, as they allow me to provide additional optimisations (leading to fewer temporaries). This could not be done with a detection-style approach as the presence of an operator+ will not tell you whether or not it is commutative. It also plays some tricks returning rvalue-references which some people don't like (or consider dangerous), so it's not suitable for Boost.Operators.

when I was looking into operators for the histogram library, I also considered Boost.Operators and I also looked into your taocpp/operators.

I was wondering why are the rvalue-optimised versions of operators are not already part of Boost.Operators? I don't know what kind of tricks you are talking about, surely there must be rvalue-enabled versions that are standard compliant and safe, like the implementations that Richard Hodges mentioned. This seems like a straight-forward improvement of Boost.Operators that doesn't break old code. Or am I missing something?

The "problem" is that returning rvalue-references will lead to dangling references in two cases. Before looking at those cases, consider a commutative operator+ (abbreviated version): T operator+( const T& lhs, const T& rhs ) { T nrv( lhs ); nrv += rhs; return nrv; } // v1 T&& operator+( T&& lhs, const T& rhs ) { lhs += rhs; return std::move( lhs ); } // v2 T&& operator+( const T& lhs, T&& rhs ) { rhs += lhs; return std::move( rhs ); } // v3, remember: we assume a commutative + T&& operator+( T&& lhs, T&& rhs ) { lhs += rhs; return std::move( lhs ); } // v4 now consider using it: const T t1, t2, t3, t4; // some values const T t = t1 + ( t2 + t3 ) + t4; The last line creates a temporary T for t2+t3 with v1, then calls v3 which calls += on the temporary, then v2 which again calls += on the temporary. Finally, the temporary is moved into t. So far, so good: Intermediate temporaries are only destroyed at the end of a full expression. This also means, that calling a function will work as expected. Given: void f( const T& ); // some function which wants above T calling f( t1 + ( t2 + t3 ) + t4 ); // fine, creates a single temporary works as it should. Now for the two cases where you can create a dangling reference: a) Explicitly binding to a reference "to prolong the lifetime of the temporary". This happens if you write const T& t = t1 + ( t2 + t3 ) + t4; As the last operator+ return an rvalue reference instead of an rvalue, the "const T& t =" does not bind to a temporary. The intermediate temporary created by (v2+v3) is destroyed at the end of the expression, hence t is now a dangling reference. Oops. This case is explicit, so you were basically asking for it. In my opinion it is user-error, since you expected a temporary but you haven't checked. Some people disagree and say that every class must make sure that this always works - basically forcing methods (especially operators) to always return an ralue / a temporary. This would effectively disallow these optimisations. As I can not know whether people used this in existing code, it is also not backwards compatible. This is the reason why I decided to put this into a new library and kept it out of Boost.Operators. b) The second case is implicit: Using a range-based for loop. Consider T being a container (or container-like) class and: for ( const auto& element : t1 + ( t2 + t3 ) + t4 ) { ... } This will also not work as the intermediate temporary created by (t2+t3) and which is passed to the range-based for loop is destroyed *before* the loop is executed. A solution is to actually store the value first: const auto t = t1 + ( t2 + t3 ) + t4; for ( const auto& element : t ) { ... } or maybe with C++20: for ( const auto t = t1 + ( t2 + t3 ) + t4; const auto& element : t ) { ... } // Yuck. Why?? I still think that a range-based for loop should treat the expression similar to a function call, the intermediate temporaries should only be destroyed at the end of the full loop. Sadly, all my attempts to convince others lead to nowhere. I hope this explains what I have done, why I chose to put it in its own library and not into Boost.Operators and what is controversial about it. Best regards, Daniel