‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, April 29, 2019 6:52 PM, Antony Polukhin via Boost
пн, 29 апр. 2019 г. в 21:08, Steven Watanabe watanabesj@gmail.com:
AMDG On 4/29/19 12:04 PM, Antony Polukhin wrote:
пн, 29 апр. 2019 г. в 20:44, Steven Watanabe via Boost boost@lists.boost.org:
On 4/29/19 11:30 AM, Antony Polukhin via Boost wrote:
I've merged a very cool optimization by Nikita Kniazev into the master branch. From now on boost::variant does pointer stealing for recursive variants. This significantly improves the performance of the variants move constructors. However if you use a variant variable after the std::move for anything except destruction and assignment then you're getting an UB. Beware!
boost::variant goes to great lengths to prevent exactly this situation. You just broke it. This change is unacceptable. Please revert it. This optimization can be used iff. you have a way to construct a valid object in the rhs.
You can restore the old slow pre-rvalue era behavior by defining BOOST_VARIANT_NO_RECURSIVE_WRAPPER_POINTER_STEALING.
That doesn't make it okay. Look, this optimization would be fine with a different variant, one that doesn't provide the never-empty guarantee. That isn't boost::variant. <...>
From the theoretical point of view I'm on your side. boost::variant is something that is never-empty.
However, from the practical point of view :
- noexcept(false) move constructor degrades performance of variant - move constructor that does a dynamic memory allocation is a surprise for the majority of users
From the teachers point of view... Boost is a collection of high quality libraries and many people look into the source codes to learn new tricks and correct approaches for solving problems. noexcept(false) move constructor that implicitly dynamically allocates is something that I would not prefer to show anyone.
From the C++ Standard Library point of view... Well, you can only assign new value or destroy a moved away variable. Everything is fine here.
This was the original argument in the thread Steven Watanabe highlighted [1]. IIRC, the problem was that boost::variant always had a never-empty guarantee, and so the "valid but unspecified state" specification for C++ moves was not met by the optimization.
3 against 1.
I see a way to restore the theoretical beauty of the variant. We can just remove the empty() member function or force it to always return false. So that variant is still never empty, but using a variant after move is an UB.
If the one negative is adding UB to valid existing code at the next point release, doesn't that outweigh any number of positive outcomes? Why wouldn't this be an opt-in strategy - this property of moves has been established for years now. Lee [1] https://lists.boost.org/Archives/boost/2013/02/200744.php