On 6 April 2018 at 09:50, Marc Glisse wrote:
> On Fri, 6 Apr 2018, Jonathan Wakely wrote:
>> This attempts to solve some of the problems when mixing std::valarray
>> operations and 'auto', by storing nested closure objects as values
>> instead of references. This means we don't end up with dangling
>> references to temporary closures that have already been destroyed.
>> This makes the closure objects introduced by the library less
>> error-prone, but it's still possible to write incorrect code by using
>> temporary valarrays, e.g.
>> std::valarray<int> f();
>> auto x = f() * 2;
>> std::valarray<int> y = x;
>> Here the closure object 'x' has a dangling reference to the temporary
>> returned by f(). It might be possible to do something about this by
>> overloading the operators for valarray rvalues and moving the valarray
>> into the closure, instead of holding a const-reference. I don't plan
>> to work on that.
>> Performance seems to be unaffected by this patch, although I didn't
>> test that very thoroughly.
>> Strictly speaking this is an ABI change as it changes the size and
>> layout of the closure types like _BinClos etc. so that their _M_expr
>> member is at least two words, not just one for a reference. Those
>> closure types should never appear in API boundaries or as class
>> members (if anybody was doing that by using 'auto' it wouldn't have
>> worked correctly anyway) so I think we can just change them, without
>> renaming the types or moving them into an inline namespace so they
>> mangle differently. Does anybody disagree?
> When I was looking into doing something similar for GMP, I had decided to
> rename the class. Some inline functions have incompatible behavior before
> and after the change, and if they are not actually inlined and you mix the 2
> types of objects (through a library for instance) without a lot of care on
> symbol visibility, a single version will be used for both.
> If you think that's too contrived a scenario, then renaming may not be
> needed.

Maybe that's safest. We can just enclose them in namespace __detail or
__valarray and then add using-declarations to add them back to
namespace std.

>> The PR is marked as a regression because the example in the PR used to
>> "work" with older releases. That's probably only because they didn't
>> optimize as aggressively and so the stack space of the expired
>> temporaries wasn't reused (ASan still shows the bug was there in older
>> releases even if it doesn't crash). As a regression this should be
>> backported, but the layout changes make me pause a little when
>> considering making the change on stable release branches.
> I wouldn't count it as a regression.

OK thanks for the comments. I think this can probably wait for stage 1.

Reply via email to