On Sunday 13 July 2014 12:06:56 Peter Dimov wrote:

Eric Niebler wrote:

...

While investigating a failure[^1] in proto, I discovered the following change to boost::ref:

https://github.com/boostorg/core/commit/af629ffa59094048c335609f285afe342f d1f1e4

This failure was caused by the following change to boost::ref by Agustín Bergé (K-ballo):

https://github.com/boostorg/core/commit/af629ffa59094048c335609f285afe342f d1f1e4

I didn't see any discussion about this change. (Was there one?) I can guess why it was made: to make boost::ref behave like std::ref, which does "reference collapsing" with reference_wrapper (which IMO is broken, and I unsuccessfully argued that in the committee when this was voted in). The problem is, it's a breaking interface change to one of the oldest, most heavily used, and stable pieces of Boost. It pretty massively breaks a major part of Boost.Proto's interface (proto::make_expr). I've hacked around the problem in Proto's tests, but this is going to break end-user code, and I don't know what I'm going to tell people.

I don't like differences between boost and std. But I also don't like breaking code. What do we want here?

We want to revert the change, unless someone (quickly) succeeds in convincing us not to.

Perhaps Eric could provide more context on why Boost.Proto needs recursive reference wrappers? Frankly, the requirement looks strange to me. I'm not strongly opposed to reverting, but the change looks justified enough to me. Allowing dangling reference wrappers is surely not the correct behavior.

On Sunday 13 July 2014 12:48:30 Peter Dimov wrote:

Andrey Semashev wrote:

...

Perhaps Eric could provide more context on why Boost.Proto needs recursive reference wrappers? Frankly, the requirement looks strange to me.

Generic code that does

boost::reference_wrapper<T> r = boost::ref( t );

is broken when t is a reference_wrapper.

Ok, but I was also interested in why this is needed by Boost.Proto. I.e. whether a reference to a reference to a reference... is the intended behavior of the library and whether it's actually desired to collapse the recursion. I understand that the library doesn't do that currently and we might need to revert the change to boost::ref temporarily to fix it for 1.56, but the long term plan might be to actually fix Boost.Proto. We might declare the old (non- collapsing) behavior deprecated for 1.56 in the release notes and provide some wrap_reference<T> trait to make the above code work: typename boost::wrap_reference<T>::type r = boost::ref(t); The trait would do a non-collapsing wrapping for 1.56 and collapse references for future releases.

...

I'm not strongly opposed to reverting, but the change looks justified enough to me. Allowing dangling reference wrappers is surely not the correct behavior.

Dangling reference wrappers are prevented by a separate and independent change

https://github.com/boostorg/core/commit/45f7564db29a3bafa5dfd8c41396843493d1 378a

that is (I think) not affected by the removal of the collapsing overloads.

Ok, good. I take it that the motivating example: boost::ref(boost::ref(t)); should not compile then?

On 13/07/2014 10:10 a.m., Agustín K-ballo Bergé wrote:

On 13/07/2014 07:16 a.m., Andrey Semashev wrote:

...

On Sunday 13 July 2014 12:48:30 Peter Dimov wrote:

...

...

I'm not strongly opposed to reverting, but the change looks justified enough to me. Allowing dangling reference wrappers is surely not the correct behavior.

Dangling reference wrappers are prevented by a separate and independent change

https://github.com/boostorg/core/commit/45f7564db29a3bafa5dfd8c41396843493d1

378a

that is (I think) not affected by the removal of the collapsing overloads.

Ok, good. I take it that the motivating example:

boost::ref(boost::ref(t));

should not compile then?

IIRC, the change was motivated by that dubious case of `boost::ref(boost::ref(t))` which was working, although not in an obvious way, and stopped working after disabling references to rvalues.

I have been going over the changes and I recall some more stuff now. Before I disabled creating references to rvalues, `boost::ref` was doing "accidental reference collapsing" for two levels of wrapping. The motivation of the change was to avoid breaking code that assumed reference collapsing as in `std::ref` and just happened to work. I would suggest consistency one way or another, but either is a breaking change. It seems the simplest solution would be to remove the reference collapsing, accidental or otherwise, and I would recommend that at least for the short run if it makes Proto work again. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com

On 13/07/2014 01:18 p.m., Peter Dimov wrote:

Agustín K-ballo Bergé wrote:

...

I have been going over the changes and I recall some more stuff now. Before I disabled creating references to rvalues, `boost::ref` was doing "accidental reference collapsing" for two levels of wrapping.

I'm not sure I understand. Can you post the code example that demonstrates the accidental collapsing before the rvalue change?

As mentioned in the pull request, cases like the following: int i= 0; boost::reference_wrapper<int> r = boost::ref(boost::ref(i)); were performing "accidental collapsing", as it triggers an implicit conversion followed by a (possibly elided) copy construction. After the rvalue change, and without explicit collapsing, this results in a compilation error. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com

On Monday 14 July 2014 10:20:41 Peter Dimov wrote:

Agustín K-ballo Bergé wrote:

...

As mentioned in the pull request, cases like the following: int i= 0; boost::reference_wrapper<int> r = boost::ref(boost::ref(i));

were performing "accidental collapsing", as it triggers an implicit conversion followed by a (possibly elided) copy construction.

Thanks.

This works because of another boost::ref 'feature', it returns a const reference_wrapper which allows the outer boost::ref to bind to it. That const return is probably a mistake, now that I think of it, but it's too late to change it now.

In fact, of the four ref/cref combinations, three happen to work as if reference collapsing overloads are present.

What makes this hard is that I think that Eric is right and that the reference collapsing overloads should never have been added to std::ref. It should be obvious that nobody would write code like the above on purpose; there's absolutely no point in calling boost::ref twice when one call would do. It's similar to something like

int x = std::cref( 5 );

which is also something that used to work and is broken now, and also something that nobody would write except by mistake.

The motivation for the reference collapsing overloads must come from examples taken from real code in which ref somehow gets applied to something that already is a reference_wrapper but not to ref(x) directly, and the code needs the result to be a flattened reference_wrapper for some reason. So far I've been unable to think of such an example. I wonder if anyone has ever encountered it. This feature looks like a typical committee invention to me.

I think Boost.Proto is one of the examples where undesired reference recursion happened. I say 'undesired' because I cannot see why somebody would want a reference to a reference. If not because of performance, conceptually there are no references to references in C++, and std::ref attempts to reflect that. Now that Boost.Proto does reference collapsing, reverting the change will break it again (I think). Do you think we could settle with a trait I proposed earlier and a prominent note in 1.56 release notes?

On Monday 14 July 2014 11:08:01 Peter Dimov wrote:

Andrey Semashev wrote:

...

I think Boost.Proto is one of the examples where undesired reference recursion happened.

Boost.Proto just uses cref in a generic context, where its argument sometimes happens to be a reference_wrapper. That's not undesired.

Generic code can still be written with the trait I proposed. What's undesired is the reference recursiveness, especially if accidental.

...

If not because of performance, conceptually there are no references to references in C++, and std::ref attempts to reflect that.

There are references to reference_wrapper though.

Yes, you can also manually instantiate reference_wrapper. Nothing's perfect. :)

...

Now that Boost.Proto does reference collapsing, reverting the change will break it again (I think).

That's possible, but I don't think so.

I suspect that you don't actually have a motivating example in mind.

That's true, I don't have a practical example at hand. But neither I can see why recursive references could be useful. With this kind of choice I prefer reference collapsing, at least because it looks more natural and std-like.

Given reference collapsing, how would you take advantage of it? What code have you written, or will write, that needs it?

In my probably uneducated view, generic libraries such as Proto or Phoenix should do reference collapsing. If not by the means of boost::ref then manually themselves. Whenever the user provides a reference_wrapper and the library intends to save it by reference (e.g. into the expression tree), reference collapsing should happen.

Do you realize that the simple use

#include

int main() { int x = 0; boost::reference_wrapper<int> r = boost::ref( x );

boost::reference_wrapper< boost::reference_wrapper<int> > r2 = boost::ref( r ); boost::reference_wrapper<int> r3 = boost::ref( r ); }

actually works without the overloads? They only serve to break r2.

I realize that the code will break but I disagree that the overloads only serve to break it. Why would you want r2? I mean, in what case would you intentionally write code to employ recursive references?

On Monday 14 July 2014 12:13:44 Peter Dimov wrote:

Andrey Semashev wrote:

...

Why would you want r2? I mean, in what case would you intentionally write code to employ recursive references?

You don't write code to "employ recursive references". You write code that applies ref to an object of type T and stores the result in reference_wrapper<T>.

Would you expect this code to work: struct my_class { void bar(); }; struct my_class_const { void bar() const; }; template< typename T > void foo(T const& t) { typedef typename boost::unwrap_reference<T>::type class_type; boost::bind(&class_type::bar, boost::ref(t))(); } foo(my_class_const()); my_class x; foo(boost::ref(x));

On Monday 14 July 2014 13:01:40 Peter Dimov wrote:

Andrey Semashev wrote:

...

Would you expect this code to work:

Unless I'm doing something wrong, this code seems to work with the old boost::ref, it also works with the new boost::ref without the collapsing overloads. I'm not sure what your point is; are you arguing that the code must fail?

Oh, right, I didn't test it, and as I was composing the code I eventually missed something. struct my_class { void bar(); }; struct my_class_const { void bar() const; }; template< typename T > void foo(T const& t) { typedef typename boost::unwrap_reference<T>::type class_type; boost::bind(&class_type::bar, boost::ref(t))(); } template< typename T > void foo1(T const& t) { foo(boost::ref(t)); } foo(my_class_const()); my_class x; foo(boost::ref(x)); foo1(boost::ref(x)); The point was that (a) unwrap_reference doesn't remove all nested levels of reference_wrappers, just the top level, so class_type does not have bar when foo1 is called and (b) boost::bind saves a reference to a reference, so get_pointer does not work as intended, not to mention that a dangling reference may be left if the function object call is delayed.

On Monday 14 July 2014 13:43:03 Peter Dimov wrote:

Andrey Semashev wrote:

...

template< typename T > void foo1(T const& t) {

foo(boost::ref(t));

}

That's getting a bit artificial now. What is the purpose of foo1 except to break the code?

Well, you might want to do something on t before invoking the common logic in foo. I admit I invented this example just now, but I can imagine something like this in real life.

...

The point was that (a) unwrap_reference doesn't remove all nested levels of reference_wrappers, just the top level, so class_type does not have bar when foo1 is called and (b) boost::bind saves a reference to a reference, so get_pointer does not work as intended, not to mention that a dangling reference may be left if the function object call is delayed.

If you delay the function call in

...

template< typename T > void foo(T const& t) {

typedef typename boost::unwrap_reference<T>::type class_type; boost::bind(&class_type::bar, boost::ref(t))();

}

it will result in a dangling reference when called with an rvalue of any

type. This line, for example:

...

foo(my_class_const());

will store a dangling reference to the my_class_const temporary.

Right. That comment about the dangling reference was more related to the call of foo1.

On 7/13/2014 2:48 AM, Peter Dimov wrote:

Andrey Semashev wrote:

...

Perhaps Eric could provide more context on why Boost.Proto needs recursive reference wrappers? Frankly, the requirement looks strange to me.

Generic code that does

boost::reference_wrapper<T> r = boost::ref( t );

is broken when t is a reference_wrapper.

Right. That's exactly what I argued in committee. Doug Gregor was the only person I was able to convince. C'est la vie. It's also why Proto broke. Proto doesn't *need* the old behavior, strictly speaking. But the part of proto that builds expressions, proto::make_expr, was designed around the old semantics. See http://bit.ly/1oUbgZr. proto::result_of::make_expr is a metafunction for computing the result of the proto::make_expr function. It interprets reference types as the types of things to be held by reference, and non-reference types to be held by value. When calling proto::make_expr at run-time, each argument to be held by reference must be wrapped in boost::ref. It uses the generic-friendly invariant of boost::ref that Peter references above. I've already hacked Proto and it's tests to accommodate the change, and the tests are turning green again, so it's not a huge deal. But end user code will have to change. Xpressive didn't break, and it looks like Spirit didn't break, so the scope of the breakage might be small. Hard to say. \e

On 7/14/2014 12:56 AM, Andrey Semashev wrote:

On Sunday 13 July 2014 17:04:09 Eric Niebler wrote:

...

On 7/13/2014 2:48 AM, Peter Dimov wrote:

...

Andrey Semashev wrote:

...

Perhaps Eric could provide more context on why Boost.Proto needs recursive reference wrappers? Frankly, the requirement looks strange to me.

Generic code that does

boost::reference_wrapper<T> r = boost::ref( t );

is broken when t is a reference_wrapper.

Right. That's exactly what I argued in committee. Doug Gregor was the only person I was able to convince. C'est la vie. It's also why Proto broke.

Proto doesn't *need* the old behavior, strictly speaking. But the part of proto that builds expressions, proto::make_expr, was designed around the old semantics. See http://bit.ly/1oUbgZr. proto::result_of::make_expr is a metafunction for computing the result of the proto::make_expr function. It interprets reference types as the types of things to be held by reference, and non-reference types to be held by value. When calling proto::make_expr at run-time, each argument to be held by reference must be wrapped in boost::ref. It uses the generic-friendly invariant of boost::ref that Peter references above.

I've already hacked Proto and it's tests to accommodate the change, and the tests are turning green again, so it's not a huge deal. But end user code will have to change. Xpressive didn't break, and it looks like Spirit didn't break, so the scope of the breakage might be small. Hard to say.

Thank you Eric.

Am I right that the user's code that uses proto::result_of::make_expr (or any other Proto metafunction for result type computation) won't be broken?

Define "broken". proto::result_of::make_expr will now compute a different type than for the previous release. So anybody who is depending on the exact type will be broken. This is probably not a lot of people.

I'm omitting for now the possibility that the user's code could use the reference wrapper like in the Peter's code sample. From your changes to Proto tests I could not see what kind of changes would be required on the user's side.

These are the changes: https://github.com/boostorg/proto/commit/d9a4fc37c2b99a08648a5245ce637e60d4b... The changes in proto/test/mem_ptr.cpp are an example of where things get weird. If you pass a reference_wrapper<int> to boost::ref, you get a reference_wrapper<int>. But it you pass it to boost::cref, you get a reference_wrapper<int const>. This changes the type computation in yet another way, and yet another code invariant is broken. Previously, it was the case that if you have a T const &, it doesn't matter if you call boost::ref or boost::cref. Now it does, and anybody who uses boost::cref together with proto::make_expr is in for a rude surprise. The type of the returned object will not be the same as the type computed by proto::result_of::make_expr. (This is what broke the test.) -- Eric Niebler Boost.org http://www.boost.org

On 07/14/2014 02:03 PM, Peter Dimov wrote:

Eric Niebler wrote:

...

These are the changes: https://github.com/boostorg/proto/commit/d9a4fc37c2b99a08648a5245ce637e60d4b...

OK. So how do we proceed from here?

I'm sitting on a commit that removes the overloads for now (for 1.56, that is). This is obviously my preference - the changes are controversial, not sufficiently motivated, and I'd have preferred for this discussion to have occurred earlier and not at release time.

Remove them. Then please send me an email, and I'll patch things up in proto/develop if need be. FWIW, I never merged the above proto commit to master, so proto is still broken there. When you delete the overloads, proto/master will be fixed. Thanks, Eric

On 7/15/2014 2:13 AM, Peter Dimov wrote:

Eric Niebler wrote:

...

On 7/14/2014 2:03 PM, Peter Dimov wrote:

...

I'm sitting on a commit that removes the overloads for now (for 1.56, that is). This is obviously my preference - the changes are controversial, not sufficiently motivated, and I'd have preferred for this discussion to have occurred earlier and not at release time.

Remove them.

OK, done.

And done on my end. Thanks.