пн, 29 апр. 2019 г. в 20:38, Steven Watanabe via Boost :

AMDG

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!

NOOOOOOOO!!!!!!

Oh yeah -- Best regards, Antony Polukhin

пн, 29 апр. 2019 г. в 20:44, Steven Watanabe via Boost :

AMDG

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. -- Best regards, Antony Polukhin

пн, 29 апр. 2019 г. в 21:08, Steven Watanabe :

AMDG

On 4/29/19 12:04 PM, Antony Polukhin wrote:

...

пн, 29 апр. 2019 г. в 20:44, Steven Watanabe via Boost :

...

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.

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. -- Best regards, Antony Polukhin

AMDG On 4/29/19 12:52 PM, Antony Polukhin wrote:

пн, 29 апр. 2019 г. в 21:08, Steven Watanabe :

...

On 4/29/19 12:04 PM, Antony Polukhin wrote:

...

пн, 29 апр. 2019 г. в 20:44, Steven Watanabe via Boost :

...

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.

I don't agree with this characterization of theoretical vs. practical. If invariants are not enforced strictly they are essentially worthless--unless you only want your program to mostly work and don't care about bugs, that is.

However, from the practical point of view : * noexcept(false) move constructor degrades performance of variant

Sacrificing correctness for performance is an unacceptable tradeoff.

* move constructor that does a dynamic memory allocation is a surprise for the majority of users

It may be a surprise, but it is something that you already need to be aware of, due to the fact that move can call the copy-constructor (or copy-assignment operator) if there is no move constructor defined.

...

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.

So we're demonstrating an incorrect solution to show people how to solve problems correctly?

...

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.

That's mostly irrelevant. The standard library provides much stronger guarantees for all of its types.

3 against 1.

Counting arguments like this is nonsensical.

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.

That sounds like the worst of all possible worlds. In Christ, Steven Watanabe

On Mon, Apr 29, 2019 at 1:44 PM Steven Watanabe via Boost wrote:

AMDG

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.

Stressing that, as Steven points out, this wouldn't violate the never-empty guarantee *if* after stealing you were able to default-construct something on the right-hand side and update the discriminator accordingly -- similar to what is done in other parts of variant. I think that if people want an alternative to this that is closer to what the change does, we'd probably want a template different from recursive_wrapper that explicitly has an empty state, leaving recursive_wrapper and variant's invariants intact. In other words, an additional template that is just an `optional` equivalent that always uses dynamic storage (at least for types that can be incomplete). This would be a different type from recursive_wrapper so as to not weaken the invariants of recursive_wrapper, but it could still be a type that variant knows about in much the same way, if that were to prove desirable. Such an optional-equivalent would be more generally useful anyway. -- -Matt Calabrese

AMDG On 4/29/19 2:28 PM, Matt Calabrese wrote:

On Mon, Apr 29, 2019 at 4:23 PM Matt Calabrese wrote:

...

<snip> Stressing that, as Steven points out, this wouldn't violate the never-empty guarantee *if* after stealing you were able to default-construct something on the right-hand side and update the discriminator accordingly

Continuing on this thought, you don't necessarily have to default-construct on the right-hand side to satisfy the never-empty guarantee here. If the left-hand side also happens to currently contain a recursive_wrapper, or something that is nothrow-move-constructible, it can be moved into or swapped with rhs.

That's true, but it doesn't help much, because it only applies to assignment, not construction. In Christ, Steven Watanabe

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Monday, April 29, 2019 7:08 PM, Antony Polukhin via Boost wrote:

пн, 29 апр. 2019 г. в 21:52, Steven Watanabe via Boost boost@lists.boost.org:

...

AMDG 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!

We've had this discussion before: https://lists.boost.org/Archives/boost/2013/02/200744.php

This discussion was considered. Please see the two comments from Nikita Kniazev https://github.com/boostorg/variant/pull/59#issuecomment-459573177

Those arguments miss the entire point - boost::variant had a long standing never-empty guarantee. So the optimization was not in a valid state according to boost::variants own specifications. Lee

AMDG On 4/29/19 1:08 PM, Antony Polukhin wrote:

<snip> This discussion was considered. Please see the two comments from Nikita Kniazev https://github.com/boostorg/variant/pull/59#issuecomment-459573177

From the PR: The nullified recursive_wrapper is in a valid but unspecified state

Strictly speaking this is true, but it is backwards incompatible because the state was not valid in previous versions.

Users may misinterpret "Never-Empty" Guarantee that was designed for throwing copy, and existed before move semantics. This is a documentation issue and can be easily be fixed.

I don't understand this. Never-Empty means Never-Empty. The meaning of the guarantee is based solely on what constitutes a valid state of a variant. How we get into the empty state has no bearing on the matter. In Christ, Steven Watanabe

-----Original Message----- From: Boost On Behalf Of Steven Watanabe via Boost Sent: 1 May 2019 03:17 To: boost@lists.boost.org Cc: Steven Watanabe Subject: Re: [boost] [variant] Heads-up! AMDG 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!

Here's my summary of why this is a terrible idea: - Boost.Variant provides the never-empty guarantee. You may disagree about whether this guarantee is useful, but it is far too late to remove a feature of an old, established component that is advertised quite loudly in the documentation. - This change breaks the never-empty guarantee because it allows a variant to be empty. Even if the changes to variant::empty were reverted, it still cannot be treated as a change solely to recursive_wrapper. A non-empty variant containing an empty recursive_wrapper would not conceptually violate variant's never empty guarantee. Unfortunately, this view is not viable due to variant's special handling of recursive_wrapper. - I find the view that there's no problem with causing undefined behavior that cannot be detected at compile time in existing code because nobody could possibly be depending on it, to be incredibly myopic. - The argument that the never-empty guarantee only applies to exception-safety and not to move is specious, because that's not how invariants work. - The old discussion of this topic concluded with a decision to leave recursive_wrapper alone and introduce a new type with the new behavior (among other things). This new change appears to completely ignore the consensus established at that time, which is one that I agree with. Nothing has changed since then, and no new arguments are adduced. +1 It is certainly against Boost tradition of avoiding breaking changes where possible. Allowing this behaviour with a new macro (and a skull'n'crossbones warning in the Boost macros documentation) might just be acceptable, but requiring a new NO_ macro to get the previous behaviour doesn't get my support FWIW. Paul Paul A. Bristow Prizet Farmhouse Kendal, Cumbria LA8 8AB UK

On Thursday, May 2, 2019, Antony Polukhin wrote:

Reverted in develop. Master will be reverted soon

Thanks Antony. Glen