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

Subject: Re: [boost] [variant] Basic rvalue and C++11 features support
From: Vicente Botet (vicente.botet_at_[hidden])
Date: 2013-01-11 08:47:11

Hartmut Kaiser wrote
>> 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).
> 
> I agree 100% with Joel. Move construction means move construction - i.e.
> the
> source object is by definition left in a zombie state. No harm done.
> What's the point in having a move constructor which essentially is
> equivalent to a copy constructor in the first place?

I have not see the code using recursive_wrapper. Do we really need that
recursive_wrapper be movable? if yes, in which cases this optimization is a
must have?

Best, 
Vicente

--
View this message in context: http://boost.2283326.n4.nabble.com/variant-Basic-rvalue-and-C-11-features-support-tp4637894p4641259.html
Sent from the Boost - Dev mailing list archive at Nabble.com.