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-wh... Sent from the Boost - Dev mailing list archive at Nabble.com.