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

Subject: Re: [boost] [variant] Basic rvalue and C++11 features support
From: Paul Smith (pl.smith.mail_at_[hidden])
Date: 2013-01-07 15:14:23

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.

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