On Mon, Jan 21, 2013 at 3:39 PM, Antony Polukhin <antoshkka@gmail.com> wrote:
Current implementation of recursive_wrapper move constructor is not optimal:
recursive_wrapper<T>::recursive_wrapper(recursive_wrapper&& operand) : p_(new T(std::move(operand.get()) )) { }
First of all, let's make a distinction here: there's recursive_wrapper, and there's a variant that contains a recursive_wrapper. Peter's solution, for example, addresses the latter, not the former.
During descussion were made following proposals:
I: Leave it as is - bad performance - can not provide noexcept guarantee
I take it as leave variant as is. I think we're already past that.
II: Set operand.p_ to NULL, add BOOST_ASSERT in get operations + good performance + provides noexcept guarantee for move constructor
recursive_wrapper's move constructor. Not necessarily variant's.
+ optimization will be used even if varinat has no type with trivial default constructor
And with a standalone recursive_wrapper.
+ easy to implement - triggers an assert when user tries to reuse moved object - adds an empty state to the recursive_wrapper
Does it? Either recursive_wrapper has a documented, valid empty state, or a "gee I hope noone ever steps here" pseudo-"state".
III: Make recursive_wrapper and variant cooperate, enable move for varinat in the presence of recursive_wrappers only when there is at least one type that is nothrow-default-constructible, regardless of whether it's current.
We can enable move construction regardless. The presence of a cheap type simply enables this optimization.
It is easyer to understand by example: typedef variant<int, recursive_wrapper<foo>> V; V v1( std::move(v2) ); This move-constructs v1 from v2 and leaves int() into v2. + good performance + provides noexcept guarantee for rcursive_wrapper
No, not for recursive_wrapper. recursive_wrapper's move constructor remains as-is. It is only when recursive_wrapper is hosted inside a variant that we can do this. And even then, if any of the other types has a throwing move/copy-ctor, variant can't have a noexcept move-ctor (which is true for the other solutions as well).
+ does not adds an empty state - optimization won't trigger if varinat has no type with trivial default constructor
And not with a standalone recursive_wrapper.
- hard to implement
Well, hardER. Variant already does something similar.
- user may be obscured by the fact, that v2 from the example now contains int
Not really. That's what a move constructor does. If the user wants copy semantics he shouldn't move the variant.
IV: After move away, delay construction of type held by recursive_wrapper till it will be be required. +/- good performance? but more checks
More checks, dynamic allocation and possibly throwing in 'get'. Not a very good thing for a previously trivial inline function. Not sure at all that this is an overall win performance-wise.
+ provides noexcept guarantee for rcursive_wrapper
And provides yesexcept for 'get' :-)
+ does not adds an explicit empty state + optimization will be used even if varinat has no type with trivial default constructor +/- not hard to implement --- additional requirement for type T of recursive_wrapper: it must be default constructible
+1
- less obvious behavior for user (for example get() function would construct values, allocate memory and can possibly throw)
Please, vote for solution or propose a better one.
Solution 2, and any of its spinoffs, is just passing responsibility from one point to the other. It's definitely not a "correct" solution. If you feel confident enough to use it regardless, fine, but let's not make this the default. For example, we can add an optional_recursive_wrapper type. (I wouldn't suggest using a macro to trigger this behavior since it can lead to ODR violations). Solution 4 is probably worse than keeping variant as it is. Other than performance implications, adding throwability to a currently trivial function is something that should be avoided. Solution 3 is the only one that plays by the rules. It's not a universal solution, but it addresses enough cases and at least for now it's good enoguh. If you're put off by the intrusiveness of it - I understand. To paraphrase this solution in a more generic way: when possible, the conservative move-ctor of variant can destructively move the contained value and construct a cheap type to cover it up. If we had a generic way to do a destructive move, then variant wouldn't need to tinker with recursive_wrapper's internals and we could apply the same optimization for user types. I'd start with just recursive_wrapper though. I'd go with 3. -- Paul Smith