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

Subject: Re: [boost] [operators] A modern SFINAE-based version of boost::operators?
From: Daniel Frey (d.frey_at_[hidden])
Date: 2017-11-14 18:16:34

> On 14. Nov 2017, at 09:43, Hans Dembinski <hans.dembinski_at_[hidden]> wrote:
> 
> Dear Daniel,
> 
>> On 13. Nov 2017, at 17:26, Daniel Frey via Boost <boost_at_[hidden]> 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