On Mon, Jan 9, 2012 at 7:29 AM, Vicente Botet <vicente.botet@wanadoo.fr>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 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.
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. [...]
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.
[I like the D-style template argument delimiters!] has_move_emulation_enabled<T> should be identical to boost::is_convertible< T&, boost::rv<T>& >, no? At least, I believe it was in previous versions of Boost.Move... I guess I should actually skim through the documentation and see what, if anything, has changed in Boost.Move! - Jeff