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.
