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-su... Sent from the Boost - Dev mailing list archive at Nabble.com.