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.
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.
--
Marc Glisse
