Re: [boost] [variant] Please vote for behavior

Subject: Re: [boost] [variant] Please vote for behavior
From: Paul Smith (pl.smith.mail_at_[hidden])
Date: 2013-02-01 09:21:48

On Fri, Feb 1, 2013 at 8:37 AM, Antony Polukhin <antoshkka_at_[hidden]> wrote:
> 2013/1/31 Paul Smith <pl.smith.mail_at_[hidden]>:
>> On Thu, Jan 31, 2013 at 9:24 AM, Antony Polukhin <antoshkka_at_[hidden]> wrote:
>>> From theoretical point of view you are absolutely correct, and my
>>> example is lame. Moreover, current implementation of move assignment
>>> and move constructors for recursive_wrapper were implemented to model
>>> that behavior.
>>
>> Just pointing out that move assignment is not affected by this
>> discussion. Everything is already allocated so it's as efficient as a
>> pointer swap.
>
> Can not agree (variant makes some additional manipulations to
> guarantee non emptyness):

I meant move-assignment of recursive_wrapper, not variant. It was
never under discussion AFAICT. Sorry if it wasn't clear.

> void move_assign(recursive_wrapper<T>&& rhs) {
> ...
> // heap allocation in move constructor of recursive_wrapper
> // can be optimized away (see #7960)
> variant temp( detail::variant::move(rhs) );
>
> // *this stores type different from recursive_wrapper<T>,
> // so it another call to move constructor of recursive_wrapper
> // will be made
> // Potential call to less effective move_assign implementation if
> // variant does not have fallback_type (+1 heap allocation and deallocation)
> variant_assign( detail::variant::move(temp) );
> ...
> // heap deallocation in destructor of recursive_wrapper
> }
>
> With recursive_ptr this would be:
>
> void move_assign(recursive_ptr<T>&& rhs) {
> ...
> // swap ptrs
> // can be optimized away (see #7960)
> variant temp( detail::variant::move(rhs) );
>
> // recursive_ptr move constructor does not throw, so
> // variant will use a fastest possible move assignment implementation
> // *this stores type different from recursive_wrapper<T>,
> // so it calls to move constructor of recursive_ptr
> //  => swap ptrs
> variant_assign( detail::variant::move(temp) );
>
> ...
> // destructors of temporaries will call delete on nullptrs
>
> // Whole function will be noexcept => variant can be move
> // assigned in STL containers (containers will use copy-assignments,
> // if this function can throw)
> }
>
> With recursive_ptr it is as efficient, as pointers swap! With
> recursive_wrapper it is multiple times slower.

IIUC you are talking about a scenario where the user assigns a
recursive_wrapper to a variant directly (i.e. some_variant =
some_recursive_wrapper_T). But this is ill-formed anyway - you can
only assign an object of the recursive_wrapper's underlying type (i.e.
some_variant = some_T).

However, there are other scenarios which you can optimize if you're up
for some extra work. When you need to back up a recursive_wrapper,
there's no need to move-construct a temporary recursive_wrapper. Just
copy and clear its pointer. Destroy the recursive_wrapper. Do the
assignment. If it succeeds, delete the pointer. If it fails,
reconstruct a recursive_wrapper using that pointer (have it take
ownership over the existing object).

And, ofcourse, Peter's optimization is equally viable for
move-assignment as it is for move-construction.

>
> 2013/2/1 Dave Abrahams <dave_at_[hidden]>:
>>
>> on Thu Jan 31 2013, Paul Smith <pl.smith.mail-AT-gmail.com> wrote:
>>
>>> On Thu, Jan 31, 2013 at 9:24 AM, Antony Polukhin <antoshkka_at_[hidden]> wrote:
>>>> From theoretical point of view you are absolutely correct, and my
>>>> example is lame. Moreover, current implementation of move assignment
>>>> and move constructors for recursive_wrapper were implemented to model
>>>> that behavior.
>>>
>>> Just pointing out that move assignment is not affected by this
>>> discussion. Everything is already allocated so it's as efficient as a
>>> pointer swap.
>>
>> Actually the correct semantics of move assignment is the same as "swap +
>> clear" if there's an empty state.  See
>> http://cpp-next.com/archive/2009/09/your-next-assignment/
>>
>> I recommend reading the whole article.
>
> Oops, if move assignment shall be equal to "swap + clear", then the
> current implementations of recursive_wrappers move assign must be
> fixed (it just swaps).

Except that recursive_wrapper doesn't have "clear". The most sensible
thing to do, if you want a truely "interference-free" move assignment,
is to propagate it down to the underlying instance (i.e. lhs.get() =
move(rhs.get()) ). Note that this can result in a copy. That's up to
you.

--
Paul Smith