[variant] Basic rvalue and C++11 features support

classic Classic list List threaded Threaded
75 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

[variant] Basic rvalue and C++11 features support

Antony Polukhin
Hi,

There is a patch in ticket #7620 that adds some rvalue assignment
operators and rvalue constructors for Boost.Variant library.

I saw no activity from authors of Boost.Variant Eric Friedman and Itay
Maman for a long time, so if there will be no objections, I'll commit
patch to trunk in the middle of November.

Any suggestions and comments are welcomed.

--
Best regards,
Antony Polukhin

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Vicente Botet
Le 01/11/12 18:57, Antony Polukhin a écrit :

> Hi,
>
> There is a patch in ticket #7620 that adds some rvalue assignment
> operators and rvalue constructors for Boost.Variant library.
>
> I saw no activity from authors of Boost.Variant Eric Friedman and Itay
> Maman for a long time, so if there will be no objections, I'll commit
> patch to trunk in the middle of November.
>
> Any suggestions and comments are welcomed.
>
>
Hi,

I have not analyzed completely the patch but I see that Boost.Move is
not used to emulate move semantics on C++98. Is there a deep reason to
don't use it?

Best,
Vicente

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Dave Abrahams

on Thu Nov 01 2012, "Vicente J. Botet Escriba" <vicente.botet-AT-wanadoo.fr> wrote:

> Le 01/11/12 18:57, Antony Polukhin a écrit :
>> Hi,
>>
>> There is a patch in ticket #7620 that adds some rvalue assignment
>> operators and rvalue constructors for Boost.Variant library.
>>
>> I saw no activity from authors of Boost.Variant Eric Friedman and Itay
>> Maman for a long time, so if there will be no objections, I'll commit
>> patch to trunk in the middle of November.
>>
>> Any suggestions and comments are welcomed.
>>
>>
> Hi,
>
> I have not analyzed completely the patch but I see that Boost.Move is
> not used to emulate move semantics on C++98. Is there a deep reason to
> don't use it?

+1
See https://svn.boost.org/trac/boost/ticket/7601

--
Dave Abrahams
BoostPro Computing                  Software Development        Training
http://www.boostpro.com             Clang/LLVM/EDG Compilers  C++  Boost


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Paul Fultz II





----- Original Message -----

> From: Dave Abrahams <[hidden email]>
> To: [hidden email]
> Cc:
> Sent: Thursday, November 1, 2012 2:53 PM
> Subject: Re: [boost] [variant] Basic rvalue and C++11 features support
>
>
> on Thu Nov 01 2012, "Vicente J. Botet Escriba"
> <vicente.botet-AT-wanadoo.fr> wrote:
>
>>  Le 01/11/12 18:57, Antony Polukhin a écrit :
>>>  Hi,
>>>
>>>  There is a patch in ticket #7620 that adds some rvalue assignment
>>>  operators and rvalue constructors for Boost.Variant library.
>>>
>>>  I saw no activity from authors of Boost.Variant Eric Friedman and Itay
>>>  Maman for a long time, so if there will be no objections, I'll
> commit
>>>  patch to trunk in the middle of November.
>>>
>>>  Any suggestions and comments are welcomed.
>>>
>>>
>>  Hi,
>>
>>  I have not analyzed completely the patch but I see that Boost.Move is
>>  not used to emulate move semantics on C++98. Is there a deep reason to
>>  don't use it?
>
> +1
> See https://svn.boost.org/trac/boost/ticket/7601

Interesting, as this will compile:

struct foo
{
    foo() {}
    boost::variant<int, float> x;
};

int main()
{
    foo a;
    a = foo();
}

But if it were to use Boost.Move, this will no longer compile, breaking barkwards compatibility.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Antony Polukhin
2012/11/2 paul Fultz <[hidden email]>:

>> From: Dave Abrahams <[hidden email]>
>>>  Hi,
>>>
>>>  I have not analyzed completely the patch but I see that Boost.Move is
>>>  not used to emulate move semantics on C++98. Is there a deep reason to
>>>  don't use it?
>>
>> +1
>> See https://svn.boost.org/trac/boost/ticket/7601
>
...
> But if it were to use Boost.Move, this will no longer compile, breaking barkwards compatibility.

Yes, main reason for not using Boost.Move's macrosses is
compatibility. To be honest, I did not know about the issue mentioned
by Paul Fultz. I was just scared with a huge amount of workarounds for
compilers with broken constructor template orderings and MSVC6
specific hacks. Using some of the Boost.Move macrosses would
definitely break compilation on some old/buggy compilers.

Compilers that do support rvalues have no such bugs (or at least all
of thouse compilers are being tested in regression tests), so allowing
move optimizations only for C++11 compilers seemed reasonable.

About ticket #7601. Using ONLY boost::move function (and NO macrosses)
can give some performance gains without risk to break compatibility
for C++03 compilers. For example, after applying patch #7620 there are
some places which could benefit if users type uses Boost.Move (for
example `move_into` visitor has code like
T(::boost::detail::variant::move(operand))). I did not used Boost.Move
in #7620 for simplicity of debugging: I did not wanted to mix possible
bugs of #7620 patch with possible bugs of #7601.


--
Best regards,
Antony Polukhin

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Vicente Botet
Le 02/11/12 08:36, Antony Polukhin a écrit :

> 2012/11/2 paul Fultz <[hidden email]>:
>>> From: Dave Abrahams <[hidden email]>
>>>>   Hi,
>>>>
>>>>   I have not analyzed completely the patch but I see that Boost.Move is
>>>>   not used to emulate move semantics on C++98. Is there a deep reason to
>>>>   don't use it?
>>> +1
>>> See https://svn.boost.org/trac/boost/ticket/7601
> ...
>> But if it were to use Boost.Move, this will no longer compile, breaking barkwards compatibility.
> Yes, main reason for not using Boost.Move's macrosses is
> compatibility. To be honest, I did not know about the issue mentioned
> by Paul Fultz. I was just scared with a huge amount of workarounds for
> compilers with broken constructor template orderings and MSVC6
> specific hacks. Using some of the Boost.Move macrosses would
> definitely break compilation on some old/buggy compilers.
>
> Compilers that do support rvalues have no such bugs (or at least all
> of thouse compilers are being tested in regression tests), so allowing
> move optimizations only for C++11 compilers seemed reasonable.
>
> About ticket #7601. Using ONLY boost::move function (and NO macrosses)
> can give some performance gains without risk to break compatibility
> for C++03 compilers. For example, after applying patch #7620 there are
> some places which could benefit if users type uses Boost.Move (for
> example `move_into` visitor has code like
> T(::boost::detail::variant::move(operand))). I did not used Boost.Move
> in #7620 for simplicity of debugging: I did not wanted to mix possible
> bugs of #7620 patch with possible bugs of #7601.
>
Hi,

I have some experience making portable move semantics for Boost.Thread
which has also its own bugged move semantics emulation. I've added my
own macros that use either the Boost.Move macros or the equivalent for
the legacy emulation. This is a transitory step as IMO the legacy should
be deprecated soon.

While I agree that the use of macros is not ideal, don't have a correct
c++03 implementation that is close to the C++11 one is a shame.

However making  a portable implementation is sometimes a headache, as
there is not a C++03 move semantic emulation for general purpose classes
as  tuple, bind, shared_ptr, ... and other TR1.

I really think that it is worth the effort and I hope that the Boost
community will finish by providing a  C++portable move semantic
emulation if we don't want that the libraries finish as is the case of
Boost.Variant, Boost.Fusion, ... providing only C++11 move semantics.

Best,
Vicente




_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Dave Abrahams
In reply to this post by Paul Fultz II

on Fri Nov 02 2012, paul Fultz <pfultz2-AT-yahoo.com> wrote:

> ----- Original Message -----
>> From: Dave Abrahams <[hidden email]>
>> To: [hidden email]
>> Cc:
>> Sent: Thursday, November 1, 2012 2:53 PM
>> Subject: Re: [boost] [variant] Basic rvalue and C++11 features support
>
>>
>>
>> on Thu Nov 01 2012, "Vicente J. Botet Escriba"
>> <vicente.botet-AT-wanadoo.fr> wrote:
>>
>>>  Le 01/11/12 18:57, Antony Polukhin a écrit :
>>>>  Hi,
>>>>
>>>>  There is a patch in ticket #7620 that adds some rvalue assignment
>>>>  operators and rvalue constructors for Boost.Variant library.
>>>>
>>>>  I saw no activity from authors of Boost.Variant Eric Friedman and Itay
>>>>  Maman for a long time, so if there will be no objections, I'll
>> commit
>>>>  patch to trunk in the middle of November.
>>>>
>>>>  Any suggestions and comments are welcomed.
>>>>
>>>>
>>>  Hi,
>>>
>>>  I have not analyzed completely the patch but I see that Boost.Move is
>>>  not used to emulate move semantics on C++98. Is there a deep reason to
>>>  don't use it?
>>
>> +1
>> See https://svn.boost.org/trac/boost/ticket/7601
>
> Interesting, as this will compile:
>
> struct foo
> {
>     foo() {}
>     boost::variant<int, float> x;
> };
>
> int main()
> {
>     foo a;
>     a = foo();
> }
>
> But if it were to use Boost.Move, this will no longer compile,
> breaking barkwards compatibility.

Oh, *that* old problem again!  I agree that it's searious.  I guess
Boost.Move is not-very-usable in that case.  I wonder if Boost.Move
could use whatever technique boost::variant does?

--
Dave Abrahams
BoostPro Computing                  Software Development        Training
http://www.boostpro.com             Clang/LLVM/EDG Compilers  C++  Boost


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Antony Polukhin
Patch from ticket #7620 was applied, passed tests and was merged to
release branch.
Next step is ticket #7712. It adds `template <class T> variant(T&&)`
constructor to Boost.Variant for compilers with rvalue references
support.

If no one objects, I'll commit it in a week.

P.S.: patch contains usage of deprecated macro, that would be fixed
before applying it.

--
Best regards,
Antony Polukhin

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Joel de Guzman
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.

Regards,
--
Joel de Guzman
http://www.boostpro.com
http://boost-spirit.com



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Mathieu Champlon-2
On 06/01/2013 01:40, 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()
>  {
>      if (p_)
>          boost::checked_delete(p_); ,
>  }
>

Hi,

Maybe I missed something but why would you test for p_ in the destructor ?
Isn't boost::checked_delete< T >( 0 ) valid ?

MAT.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Joel de Guzman
On 1/7/13 2:04 AM, Mathieu Champlon wrote:
> On 06/01/2013 01:40, Joel de Guzman wrote:

>>  template <typename T>
>>  recursive_wrapper<T>::~recursive_wrapper()
>>  {
>>      if (p_)
>>          boost::checked_delete(p_); ,
>>  }
>
> Hi,
>
> Maybe I missed something but why would you test for p_ in the destructor ?
> Isn't boost::checked_delete< T >( 0 ) valid ?

You are right. The check is not needed.

Regards,
--
Joel de Guzman
http://www.boostpro.com
http://boost-spirit.com



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Alec Ross
In message <[hidden email]>, Joel de Guzman
<[hidden email]> writes

>On 1/7/13 2:04 AM, Mathieu Champlon wrote:
>> On 06/01/2013 01:40, Joel de Guzman wrote:
>
>>>  template <typename T>
>>>  recursive_wrapper<T>::~recursive_wrapper()
>>>  {
>>>      if (p_)
>>>          boost::checked_delete(p_); ,
>>>  }
>>
>> Hi,
>>
>> Maybe I missed something but why would you test for p_ in the destructor ?
>> Isn't boost::checked_delete< T >( 0 ) valid ?
>
>You are right. The check is not needed.
>

But is there an expected performance benefit?

Regards,
--
Alec Ross

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Paul Smith
In reply to this post by Joel de Guzman
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
Reply | Threaded
Open this post in threaded view
|

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

Joel de Guzman
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).

Regards,
--
Joel de Guzman
http://www.boostpro.com
http://boost-spirit.com



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Hartmut Kaiser
> 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




_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Eric Niebler-3
In reply to this post by Paul Smith
On 1/7/2013 12:14 PM, Paul Smith wrote:
> 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.

Paul, I think you are confused about move semantics. Joel and Hartmut
are right. Nobody should ever be seeing a moved-from object. Move
constructors and assignment operators exist precisely to enable this
kind of optimization.

--
Eric Niebler
BoostPro Computing
http://www.boostpro.com

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Paul Smith
In reply to this post by Joel de Guzman
On Tue, Jan 8, 2013 at 2:04 AM, Joel de Guzman <[hidden email]> wrote:

> On 1/8/13 4:14 AM, Paul Smith wrote:
>>
>> 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 hear ya. However, the C++11 take on move semantics is, for better
and for worse, a conservative one and not a destructive one. It's a
common misconception that a moved-from object is "as good as dead" and
that the move constructor can perform an autopsy. (It took me a while
to realize that, too).

A move constructor has merely a "license to mutate" the source object
while keeping it's invariants, not a "license to kill". This is, I
believe, the intended convention. This is more or less what the STL
expect from client types. And, even though it's not enforced by the
core language, the effective semantics of rvalues and
rvalue-references are such that a destructive move is unsafe. In
particular, subobjects of rvalues are rvalues of the same value
category, and can equally well bind to rv-refs. Even if a top-level
rvalue is immediately destroyed after one of its subojects has been
moved-from, its destructor might still attempt to use that subobject.
If a type gives me public access to one of it's subobjects, it means
that I'm allowed to mutate it using it's public interface any way I
want, but nothing more. If the subobject's move consutrctor (which I
can rightfully invoke) does something that's impossible to acheive
using the public interface, I'm breaking this contract, and even if I
no longer use the containing object, I might get punished when it's
destructor takes place:

class base {
  void something_naughty(); // don't attempt to use the
                                             // object after calling me
public:
  base();
  base(base&& other) {other.something_naughty();}

  void something_ordinary();
};

struct derived : base {
  ~derived() {something_ordinary();}
};

int main() {
  base b = derived(); // I'm only using the public interface
                                 // of derived. How did I manage to
                                 // screw something up?
}

Note that this also applies to data members.
Also, note that rvalues can ofcourse be std::moved lvalues, in which
case they can be accessed by the programmer after having moved them,
and he might wish to reuse them (or, if the lvalue is an lv-ref
parameter, he might not know if whoever passed him this lvalue does),
even though, admittedly, I haven't seen many convincing examples of
such cases.

I agree that the cases for which a destructive move is appropriate
seem by far the dominant ones (even if it's no different than a
conservative move in many of them). If you're asking for a rationale
as to why the semantics are the way they are, then you're talking to
the wrong person. I am sure David Abrahams can give a much more
credible answer than I can. Some of these issues are discussed in
N1377, in particular, see the "Alternative Move Designs" ->
"Destructive move semantics" section
(http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2002/n1377.htm#Alternative
move designs)
(Also note that the "Copy vs Move" section literally states: "The only
requirement (from the move-ctor PS) is that the (source PS) object
remain in a self consistent state (all internal invariants are still
intact).")

FWIW, I'm playing with the idea of writing a destructive-move proposal
(although what I'm reffering to as destructive-move is a bit different
than what the move proposal calls a destructive-move), and these kind
of cases are exactly what I wish to address. If you have actual
numbers that demonstrate how bad is the impact on Spirirt and co, I'd
love to hear them.

>
>
> Regards,
> --
> Joel de Guzman
> http://www.boostpro.com
> http://boost-spirit.com
>
>
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost

--
Paul Smith

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Paul Smith
In reply to this post by Hartmut Kaiser
On Tue, Jan 8, 2013 at 2:49 AM, Hartmut Kaiser <[hidden email]> wrote:

>> On 1/8/13 4:14 AM, Paul Smith wrote:
>> > 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?

Because it's not equivalent to a copy constructor. I can mutate the
source object, just not break it. The move-ctor of std::vector is much
more efficient than the copy-ctor, even though it leaves the source as
a completely valid vector. Even in the recursive_wrapper case, the
move-ctor is still (potentially) more efficient than the copy-ctor.

>
> Regards Hartmut
> ---------------
> http://boost-spirit.com
> http://stellar.cct.lsu.edu
>
>
>
>
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

--
Paul Smith

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Michael Caisse-3
In reply to this post by Joel de Guzman
On 01/05/2013 05:40 PM, 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):
>

<snip>

>
> Thoughts? I can commit this patch if it is acceptable.
>
> Regards,

Hi Joel -

I think the patch looks good and I would be extremely happy to see it
adopted. I have several libraries that are heavy users of
recursive_wrapper and would benefit from this change.

Does variant have an active maintainer?

michael

--
Michael Caisse
ciere consulting
ciere.com

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Joel de Guzman
In reply to this post by Paul Smith
On 1/8/13 11:05 AM, Paul Smith wrote:

> On Tue, Jan 8, 2013 at 2:04 AM, Joel de Guzman <[hidden email]> wrote:
>> On 1/8/13 4:14 AM, Paul Smith wrote:
>>>
>>> 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 hear ya. However, the C++11 take on move semantics is, for better
> and for worse, a conservative one and not a destructive one. It's a
> common misconception that a moved-from object is "as good as dead" and
> that the move constructor can perform an autopsy. (It took me a while
> to realize that, too).
>
> A move constructor has merely a "license to mutate" the source object
> while keeping it's invariants, not a "license to kill". This is, I
> believe, the intended convention. This is more or less what the STL
> expect from client types. And, even though it's not enforced by the
> core language, the effective semantics of rvalues and
> rvalue-references are such that a destructive move is unsafe. In
> particular, subobjects of rvalues are rvalues of the same value
> category, and can equally well bind to rv-refs. Even if a top-level
> rvalue is immediately destroyed after one of its subojects has been
> moved-from, its destructor might still attempt to use that subobject.
> If a type gives me public access to one of it's subobjects, it means
> that I'm allowed to mutate it using it's public interface any way I
> want, but nothing more. If the subobject's move consutrctor (which I
> can rightfully invoke) does something that's impossible to acheive
> using the public interface, I'm breaking this contract, and even if I
> no longer use the containing object, I might get punished when it's
> destructor takes place:
>
> class base {
>    void something_naughty(); // don't attempt to use the
>                                               // object after calling me
> public:
>    base();
>    base(base&& other) {other.something_naughty();}
>
>    void something_ordinary();
> };
>
> struct derived : base {
>    ~derived() {something_ordinary();}
> };
>
> int main() {
>    base b = derived(); // I'm only using the public interface
>                                   // of derived. How did I manage to
>                                   // screw something up?
> }
>
> Note that this also applies to data members.
> Also, note that rvalues can ofcourse be std::moved lvalues, in which
> case they can be accessed by the programmer after having moved them,
> and he might wish to reuse them (or, if the lvalue is an lv-ref
> parameter, he might not know if whoever passed him this lvalue does),
> even though, admittedly, I haven't seen many convincing examples of
> such cases.
>
> I agree that the cases for which a destructive move is appropriate
> seem by far the dominant ones (even if it's no different than a
> conservative move in many of them). If you're asking for a rationale
> as to why the semantics are the way they are, then you're talking to
> the wrong person. I am sure David Abrahams can give a much more
> credible answer than I can. Some of these issues are discussed in
> N1377, in particular, see the "Alternative Move Designs" ->
> "Destructive move semantics" section
> (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2002/n1377.htm#Alternative
> move designs)
> (Also note that the "Copy vs Move" section literally states: "The only
> requirement (from the move-ctor PS) is that the (source PS) object
> remain in a self consistent state (all internal invariants are still
> intact).")
>
> FWIW, I'm playing with the idea of writing a destructive-move proposal
> (although what I'm reffering to as destructive-move is a bit different
> than what the move proposal calls a destructive-move), and these kind
> of cases are exactly what I wish to address. If you have actual
> numbers that demonstrate how bad is the impact on Spirirt and co, I'd
> love to hear them.

Those are very good points, but I think you are being too pedantic.
The example you posted concerns a class hierarchy (base / derived).
The paper you cited mentions:

   "When dealing with class hierarchies, destructive move semantics
   becomes problematic."

That is certainly not the case with recursive_wrapper. Do you see
a reasonable cause for danger with (a non-hierarchical struct such
as) recursive_wrapper?

Finally, it would be best if you quote from the standard. That paper
you posted is quite old (from 2002).

Regards,
--
Joel de Guzman
http://www.boostpro.com
http://boost-spirit.com



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
1234