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 Botet (vicente.botet_at_[hidden])
Date: 2012-01-09 10:29:26

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_)) 

Why the following (both) overloads are needed?

            thread_data(BOOST_RV_REF(F) f_);
            thread_data(F const& f_); 

> 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?

> 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.
 

> 2) SFINAE requiredBoost.Move requires SFINAE, and by extension
> Boost.Thread now also requires SFINAE. Previously, Boost.Thread did
> not require SFINAE. Is this an acceptable regression, or should more
> work be put into maintaining compatibility with compilers that do not
> support SFINAE?
> 

A lot of libraries are requiring SFINAE now. Could you create a Boost.Move
ticket if the limitation is not documented and a ticket doesn't exists
already for this issue?

We could have a specific code for compilers that don't support SFINAE. I
will see what I can do while merging your patch.

> 3) Not using has_move_emulation_enabledI did not notice the
> `has_move_emulation_enabled` metafunction, and instead used
> `is_convertible!(T&, boost::rv!(T))`. This may mean that some
> situations that `has_move_emulation_enabled` accounts for will not be
> accounted for by the code in the patch. I have not yet evaluated
> whether simply replacing all the occurrences will work. Any thoughts?
> 

I have no idea. I suspect that I will need to look for this in Boost.Move.

> 4) Not using Vicente's patches for #6174 or #5990By the time that I
> was aware of these patches, I had already independently fixed the
> issues. I have not looked at either patch in detail, but they each go
> about fixing the issues in a significantly different way from my
> patch. Which code should be used in each of these cases?
> 

Yes, I know that some of my commits could be in conflict with the Boost.Move
adoption, but I wanted to fix as much as possible as I was not sure the
adoption will be in release 1.49. I will take a lot of care while doing the
merge.

> Where should I go from here? I could do the integration into the svn
> trunk version, write the documentation, or update the tests to the new
> version. Are there any other tasks that I can help out with? I will of
> course keep the patch updated with fixes for any bugs that I find in
> it.
> 

Could you try to run the test on trunk related to move semantics with your
version? To do that just copy
the directories threads and sync from libs/thread/test and the Jamfile
whithout changing the sources and do

bjam threads sync

I will commit this evening more updates on trunk so that these tests should
run better.

Could you analyze in more detail the impact of using
has_move_emulation_enabled?

Thanks a lot,
Vicente

--
View this message in context: http://boost.2283326.n4.nabble.com/thread-Boost-Thread-defines-boost-move-which-conflicts-with-Boost-Move-tp4249499p4278761.html
Sent from the Boost - Dev mailing list archive at Nabble.com.