Re: [boost] [variant] Basic rvalue and C++11 features support

Subject: Re: [boost] [variant] Basic rvalue and C++11 features support
From: Joel de Guzman (djowel_at_[hidden])
Date: 2013-01-07 19:04:48

On 1/8/13 4:14 AM, Paul Smith wrote:
> Joel de Guzman wrote:
>> Hi,
>>
>> I just looked at the current state of variant and noticed that the
>> implementation
>> of recursive_variant's move ctor seems to be not as optimal as I hoped.
>> The
>> current implementation as written is:
>>
>>    recursive_wrapper<T>::recursive_wrapper(recursive_wrapper&& operand)
>>      : p_(new T( detail::variant::move(operand.get()) ))
>>   {
>>   }
>>
>> Unless I am mistaken, I think the heap allocation is not needed
>> and the target should simply grab the pointer and mark the source
>> pointer as 0 (with additional checks for 0 in the dtor):
>>
>>   template <typename T>
>>   recursive_wrapper<T>::recursive_wrapper(recursive_wrapper&& operand)
>>       : p_(operand.release())
>>   {
>>   }
>>
>>   template <typename T>
>>   T* recursive_wrapper<T>::release()
>>   {
>>       T* ptr = p_;
>>       p_ = 0;
>>       return ptr;
>>   }
>>
>>   template <typename T>
>>   recursive_wrapper<T>::~recursive_wrapper()
>>   {
>>       if (p_)
>>           boost::checked_delete(p_);
>>   }
>>
>> The tests are running just fine (tested with gcc, msvc) with this
>> (more optimal) implementation. I also run the Spirit test suite
>> (Spirit makes heavy use of variant).
>>
>> Thoughts? I can commit this patch if it is acceptable.
>
>
> A recursive_wrapper is not a pointer. It's a value-like wrapper
> that is assumed to always contain a valid object. The move
> constructor should leave the moved-from recursive_wrapper
> in a valid state, which precludes nullifying it.
> That is, unless you suggest adding an "empty" state to
> recursive_wrapper, which doesn't sound like a very good
> idea.

I disagree. That state will happen only when copying rvalues which
will immediately be destructed anyway. What danger do you see in
that situation? Example:

    recursive_wrapper<foo> bar() {...} // function returning recursive_wrapper

    recursive_wrapper<foo> foo(bar()); // copy

Under no circumstances will anyone get to see that "empty" state.
Do you see something that I don't?

Without this move optimization (as it currently is), it is very inefficient
especially with big structures (e.g. tuples and fusion adapted structs).
Without this optimization, such temporary copies will end up with two
heap allocations and unnecessary copying of the structures, instead of
one heap allocation and a simple pointer swap. That would mean the missed
optimization in the order of magnitudes with applications that use variant
heavily (e.g. Spirit).

Regards,
-- 
Joel de Guzman
http://www.boostpro.com
http://boost-spirit.com