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-09 17:17:26

Le 09/01/12 22:31, Jeffrey Lee Hellrung, Jr. a écrit :
> On Mon, Jan 9, 2012 at 7:29 AM, Vicente Botet<vicente.botet_at_[hidden]>wrote:
>
>> Mankarse wrote
>>> On Thu, 05 Jan 2012 07:00:27 +0100, Vicente J. Botet Escriba
>>> wrote:>Mankarse please, could you post here the patch for the
>>> boost.thread>class so that I can start the integration of what you
>>> have already done?
>>> I have attached a patch to the ticket
>>> (https://svn.boost.org/trac/boost/ticket/6194). This patch is fairly
>>> comprehensive in terms of code, but it does not include any tests or
>>> documentation. In the course of writing the patch, I created some
>>> simple tests, but I have not yet translated these into Boost.Test
>>> testcases, or included them in the patch.
>>>
>> Thanks Evan,
>>
>> I will try to merge it on my own repository and see how it behaves. I have
>> added some tests to check the move semantics. They are in directory
>> libs/thread/test/threads and libs/thread/test/sync. Maybe you could check
>> them and suggest for others.
>>
>> BTw,
>>
>> 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_))
>>
> I don't think so. I haven't brushed up on the latest changes to Boost.Move
> since being officially added to Boost, but prior incarnations paired
> boost::forward with BOOST_FWD_REF. I think boost::move is correct when
> using BOOST_RV_REF.
>
>
>> 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 :( If you don't want to compromise, you could use a different set
> of overloads in C++03 and C++11:
>
> #ifndef BOOST_NO_RVALUE_REFERENCES
> template<  class F>  thread_data(F&&  f); // and use boost::forward<F>(f)
> #else // #ifndef BOOST_NO_RVALUE_REFERENCES
> template<  class F>  thread_data(F f); // and use boost::move(f)
> template<  class F>  thread_data(boost::rv<F>&  f); // and use boost::move(f)
> #endif // #ifndef BOOST_NO_RVALUE_REFERENCES
>
> 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.
I have not used Boost.Move yet. I will take this in account while 
applying the patch.
>
>>> I have tested the basic functionality on a number of compilers, but
>>> there are probably still some bugs in the patched code.
>>> The patch contains a number of known problems that may be worth
>>> discussing:1) Explicit move() and copy() calls:boost::thread and
>>> boost::packaged_task both have templated constructors that take an
>>> rvalue reference to a functor (which they move or copy as
>>> appropriate). Templates parameters cannot be deduced to a type that is
>>> only reachable through an conversion operator. Template constructors
>>> can also never be explicitly instansiated. (As far as I know.) This
>>> means that the overloads that are used in the definitions of
>>> BOOST_COPYABLE_AND_MOVABLE types (BOOST_RV_REF(type),
>>> BOOST_CATCH_CONST_RLVALUE(type), type&), are not matched for rvalue
>>> objects (in C++03 mode). Example:
>>>      //In C++11 the `CopyableAndMovableFunctor` would be    //moved
>>> into the thread.    //In C++03 this does not compile.
>>> boost::thread((CopyableAndMovableFunctor()))
>>>
>> 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?
>>
> It's a known limitation in Boost.Move, AFAIK. The above change should
> address this issue.
>
Could you point where in the doc it is described or to the Trac ticket?
>>> Here, no overload would be matched, because the `type` in the
>>> overloads is templated. To solve this problem I modified Boost.Move to
>>> add two member functions - `copy()` and `move()` - which forward to
>>> the rv const&  and rv&  conversion operators. With these, the above
>>> example can be rewritten as:
>>>      //In both C++11 and C++03 the `CopyableAndMovableFunctor`
>>> //will be moved or copied into the thread as appropriate.
>>> boost::thread(CopyableAndMovableFunctor().move()),
>>> boost::thread(CopyableAndMovableFunctor().copy())
>>> I can imagine the names `copy` and `move` being quite common, so this
>>> might cause unfortunate conflicts. Is this an acceptable change to
>>> Boost.Move? Is there another way to get the functionality that does
>>> not require this?
>>>
>> 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?
>>
>> 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 acceptable if the user interface doesn't changes.

Thanks for all your comments,
Vicente