Re: [boost] [thread] Boost.Thread defines boost::move which conflicts with Boost.Move

Subject: Re: [boost] [thread] Boost.Thread defines boost::move which conflicts with Boost.Move
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2012-01-16 13:51:21

Le 10/01/12 13:11, Evan Wallace a écrit :
>> Should't the following
>>             thread_data(BOOST_RV_REF(F) f_):
>> 		                f(boost::move(f_))
>>
>> use forward instead of move?
>>             thread_data(BOOST_RV_REF(F) f_):
>> 		                f(boost::forward<F>(f_))
>>
>> Why the following (both) overloads are needed?
>>
>>             thread_data(BOOST_RV_REF(F) f_);
>>             thread_data(F const&  f_);
>>
> When I submitted the patch, I thought that my handling of the forwarding was a
> bit awkward. As I will  describe later in this email, I have submitted a new
> patch which uses the technique suggested by Jeffrey Lee Hellrung. That said,
> this discussion about the `thread_data` constructors is mostly irrelevant.
> `thread_data` is an internal type, and the code in the `thread` constructors
> already performs the lvalue/rvalue matching and calls the appropriate
> `thread_data` constructor.
>
>> Hrr, this is really a bad new. Could you create a Boost.Move ticket if the
>> limitation is not documented and a ticket doesn't exists already for this
>> issue?
> As the issue has been resolved by an alternative technique, this is probably not
> necessary.
>
>> Could you try to run the test on trunk related to move semantics with your
>> version?
> I will do this later tonight.
>
>> Could you analyze in more detail the impact of using
>> has_move_emulation_enabled?
> I investigated further, and while I still do not fully understand the impact, it
> seems that it is harmless to use it in the particular case where I am using it.
> In the name of consistency, I have changed the code to use
> has_move_emulation_enabled in version 1 of the patch.
>
>>> Why the following (both) overloads are needed?
>>>
>>>             thread_data(BOOST_RV_REF(F) f_);
>>>             thread_data(F const&  f_);
>> I don't know the context here, but I believe, in C++03, this would:
>> capture-and-copy lvalues and rvalues; and capture-and-move explicitly
>> created emulated rvalue references. Thus, true rvalues are captured as
>> lvalues :(
> `thread_data` is an internal class. Its constructors are only ever called with
> explicitly created rvalues (which are forwarded from the rvalues that are
> matched in the constructor of `thread`), or lvalues. Thus the above behaviour is
> not problematic.
>
>> If you don't want to compromise, you could use a different set
>> of overloads in C++03 and C++11:
>> [snip]
>> I haven't tested this, but it *should* capture-and-move both true rvalues
>> and emulated rvalue references in C++03. And if it's an emulated rvalue
>> reference, the second overload requires 1 fewer move construction.
> Thankyou for the idea!
>
>> It's a known limitation in Boost.Move, AFAIK. The above change should
>> address this issue.
> It does. I have submitted a new version of the patch that uses those overloads
> in the constructors of `thread` and `packaged_task`.
>
>>> [snip... explicit move() and copy() ...]
>> I don't like this too much. Why doesn't boost::move works in this case?
>> Could you give an example on which .copy() is needed?
> boost::move will not work on rvalues. `.copy()` is needed to allow a constructor
> to be matched from a const rvalue.
>
>>> I don't know if this point will prevent the adoption of Boost.Move by
>>> Boost.Thread. I will comeback when I do some trials.
>> See if the aforementioned solution I gave is acceptable.
> It is. Thankyou!
>
>> [I like the D-style template argument delimiters!]
> I have no confidence in my mail client whatsoever (you can see what it did to
> the formatting of my message), and I was trying to avoid the template being
> argument delimiters being seen as HTML tags and stripped. Note also - there was
> an error in my original posting, what I meant was
> `is_convertible!(T&, boost::rv!(T)&)`.
>
>> has_move_emulation_enabled<T>  should be identical to
>> boost::is_convertible<T&, boost::rv<T>&  >, no?
> It isn't identical, but in the particular situation in which it is being used, I
> believe it has the same effect.
>
>> If the above is all accurate, then I wonder where the above 3 constructor
>> overloads came from in the first place...?
> The above is all accurate. The 3 constructor overloads originally came from the
> constructors that all BOOST_COPYABLE_AND_MOVABLE types have. I thought that
> using the same overloads would also be appropriate for a template. I was wrong
> (for the reasons that you gave).
>
Hi,

I have adapted your patch and many other tickets and committed the whole 
on trunk at revision 76543.

* [@http://svn.boost.org/trac/boost/ticket/2741 #2741] Proposal to 
manage portable and non portable thread attributes.
* [@http://svn.boost.org/trac/boost/ticket/6195 #6195] c++11 compliance: 
Provide the standard time related interface using Boost.Chrono.
* [@http://svn.boost.org/trac/boost/ticket/6224 #6224] c++11 compliance: 
Add the use of standard noexcept on compilers supporting them.
* [@http://svn.boost.org/trac/boost/ticket/6226 #6226] c++11 compliance: 
Add explicit bool conversion from locks.
* [@http://svn.boost.org/trac/boost/ticket/6230 #6230] c++11 compliance: 
Follows the exception reporting mechanism as defined in the c++11.
* [@http://svn.boost.org/trac/boost/ticket/6272 #6272] c++11 compliance: 
Add thread::id hash specialization.
* [@http://svn.boost.org/trac/boost/ticket/6273 #6273] c++11 compliance: 
Add cv_status enum class and use it on the conditions wait functions.
* [@http://svn.boost.org/trac/boost/ticket/6194 #6194] Adapt to Boost.Move.

Fixed Bugs:

* [@http://svn.boost.org/trac/boost/ticket/2575 #2575] Bug- Boost 1.36.0 
on Itanium platform.
* [@http://svn.boost.org/trac/boost/ticket/4921 #4921] 
BOOST_THREAD_USE_DLL and BOOST_THREAD_USE_LIB are crucial and need to be 
documented.
* [@http://svn.boost.org/trac/boost/ticket/5013 #5013] documentation: 
boost::thread: pthreas_exit causes terminate().

* [@http://svn.boost.org/trac/boost/ticket/5351 #5351] interrupt a 
future get boost::unknown_exception.
* [@http://svn.boost.org/trac/boost/ticket/5516 #5516] Upgrade lock is 
not acquired when previous upgrade lock releases if another read lock is 
present.
* [@http://svn.boost.org/trac/boost/ticket/5990 #5990] 
shared_future<T>::get() has wrong return type.
* [@http://svn.boost.org/trac/boost/ticket/6174 #6174] packaged_task 
doesn't correctly handle moving results.

I have tested it on Mac gcc and clang, Windows MSVC-10, MinGw gcc-4.6.0 0x.

I have some trouble yet on thread constructors.
Hopping I have not introduced other regressions on other platforms.

Best,
Vicente