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: Jeffrey Lee Hellrung, Jr. (jeffrey.hellrung_at_[hidden])
Date: 2012-01-09 16:31:11

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