Re: [boost] [variant] Please vote for behavior (Was: Basic rvalue and C++11 features seupport)

Subject: Re: [boost] [variant] Please vote for behavior (Was: Basic rvalue and C++11 features seupport)
From: Paul Smith (pl.smith.mail_at_[hidden])
Date: 2013-01-21 11:33:14

On Mon, Jan 21, 2013 at 3:39 PM, Antony Polukhin <antoshkka_at_[hidden]> wrote:
> Current implementation of recursive_wrapper move constructor is not optimal:
>
> recursive_wrapper<T>::recursive_wrapper(recursive_wrapper&& operand)
>     : p_(new T(std::move(operand.get()) ))
> { }

First of all, let's make a distinction here: there's
recursive_wrapper, and there's a variant that contains a
recursive_wrapper. Peter's solution, for example, addresses the
latter, not the former.

>
> During descussion were made following proposals:
>
> I: Leave it as is
>   - bad performance
>   - can not provide noexcept guarantee

I take it as leave variant as is. I think we're already past that.

>
> II: Set operand.p_ to NULL, add BOOST_ASSERT in get operations
>   + good performance
>   + provides noexcept guarantee for move constructor

recursive_wrapper's move constructor. Not necessarily variant's.

>   + optimization will be used even if varinat has no type with trivial
> default constructor

And with a standalone recursive_wrapper.

>   + easy to implement
>   - triggers an assert when user tries to reuse moved object
>   - adds an empty state to the recursive_wrapper

Does it? Either recursive_wrapper has a documented, valid empty state,
or a "gee I hope noone ever steps here" pseudo-"state".

>
> III: Make recursive_wrapper and variant cooperate, enable move for
> varinat in the presence of recursive_wrappers only when there is at
> least one type that is nothrow-default-constructible, regardless of
> whether it's current.

We can enable move construction regardless. The presence of a cheap
type simply enables this optimization.

> It is easyer to understand by example:
>         typedef variant<int, recursive_wrapper<foo>> V;
>         V v1( std::move(v2) );
>         This move-constructs v1 from v2 and leaves int() into v2.
>   + good performance
>   + provides noexcept guarantee for rcursive_wrapper

No, not for recursive_wrapper.
recursive_wrapper's move constructor remains as-is. It is only when
recursive_wrapper is hosted inside a variant that we can do this. And
even then, if any of the other types has a throwing move/copy-ctor,
variant can't have a noexcept move-ctor (which is true for the other
solutions as well).

>   + does not adds an empty state
>   - optimization won't trigger if varinat has no type with trivial
> default constructor

And not with a standalone recursive_wrapper.

>   - hard to implement

Well, hardER. Variant already does something similar.

>   - user may be obscured by the fact, that v2 from the example now contains int

Not really. That's what a move constructor does. If the user wants
copy semantics he shouldn't move the variant.

>
> IV: After move away, delay construction of type held by
> recursive_wrapper till it will be be required.
>   +/- good performance? but more checks

More checks, dynamic allocation and possibly throwing in 'get'. Not a
very good thing for a previously trivial inline function. Not sure at
all that this is an overall win performance-wise.

>   + provides noexcept guarantee for rcursive_wrapper

And provides yesexcept for 'get' :-)

>   + does not adds an explicit empty state
>   + optimization will be used even if varinat has no type with trivial
> default constructor
>   +/- not hard to implement
>   --- additional requirement for type T of recursive_wrapper: it must
> be default constructible

+1

>   - less obvious behavior for user (for example get() function would
> construct values, allocate memory and can possibly throw)
>
>
> Please, vote for solution or propose a better one.

Solution 2, and any of its spinoffs, is just passing responsibility
from one point to the other. It's definitely not a "correct" solution.
If you feel confident enough to use it regardless, fine, but let's not
make this the default. For example, we can add an
optional_recursive_wrapper type. (I wouldn't suggest using a macro to
trigger this behavior since it can lead to ODR violations).

Solution 4 is probably worse than keeping variant as it is. Other than
performance implications, adding throwability to a currently trivial
function is something that should be avoided.

Solution 3 is the only one that plays by the rules. It's not a
universal solution, but it addresses enough cases and at least for now
it's good enoguh.
If you're put off by the intrusiveness of it - I understand. To
paraphrase this solution in a more generic way: when possible, the
conservative move-ctor of variant can destructively move the contained
value and construct a cheap type to cover it up. If we had a generic
way to do a destructive move, then variant wouldn't need to tinker
with recursive_wrapper's internals and we could apply the same
optimization for user types. I'd start with just recursive_wrapper
though.

I'd go with 3.

--
Paul Smith