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

Subject: Re: [boost] [variant] Basic rvalue and C++11 features support
From: Hartmut Kaiser (hartmut.kaiser_at_[hidden])
Date: 2013-01-07 19:49:31

> 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?

Regards Hartmut
---------------
http://boost-spirit.com
http://stellar.cct.lsu.edu