Re: [boost] [Review] Formal Review: Boost.Move

Subject: Re: [boost] [Review] Formal Review: Boost.Move
From: Jeffrey Lee Hellrung, Jr. (jhellrung_at_[hidden])
Date: 2010-05-24 23:15:47

On 5/10/2010 3:24 PM, OvermindDL1 wrote:
> Greetings Boost Developers and Users,
>
> It's my pleasure to announce that the review of Ion Gaztañagas' Move
> library starts today, May 10th and lasts until May 24th, 2010, unless
> an extension occurs.
[...]

I believe Ion's Move library should be accepted in some form or another. 
  I'm obviously hoping all of the following issues can be addressed, but 
I definitely understand if they cannot be right away (priorities...) or 
are flat-out rejected ;)  I think this library is extremely important, 
at least in the short-to-middle-term (until rvalue references are 
sufficiently prevalent).

There is a lot of ingenuity in making the move emulation work, and Ion 
has made it very accessible and managed to make differences between 
C++03 and C++0x vanish behind macros and free functions.  This is great, 
and I only wish to point out some areas where I think things can be 
improved.

I suggest the following changes should be effected prior to release.  If 
you want elaboration, feel free to ask or reference the archives.

--------

- Document the existence of boost::rv<T> and that boost::rv<T>& 
represents an emulated rvalue reference in C++03.  Define the 
requirements a class must satisfy to interoperate with the move 
machinery, especially in C++03.  I don't know what these should be, 
precisely, or if they necessarily need to align with the standard 
concepts of Movable/MoveConstructible/MoveAssignable/whatever, but I did 
give an idea of what such a requirement might look like in a previous 
email.  I'm willing to bat ideas around regarding this.

- Relegate is_movable to only be used in C++03, and document its use as 
such.  I'm fine with the name "is_movable", although I understand it 
isn't in line with the standard's concept of movable, so I would 
understand a name change...has_move_emulation_enabled? ;)

- I remember why I needed an is_class within the def'n of is_movable: so 
that is_movable< X& > evaluated to false for class types X (instead of 
giving a compiler error).  Thus, I think the def'n of is_movable should 
be something like

template< class T >
struct is_movable
     : mpl::and_< is_class<T>, mpl::not_< is_rvalue_reference<T> >, 
is_convertible< T, rv<T>& > >
{ };

- Either a) provide 2 separate macros to enable move emulation, one 
"safe"/"friendly" and the other "optimal"; or b) provide a single macro 
to enable "safe"/"friendly" (i.e., non-optimal) move emulation.  I 
think, if we're going to package 2 possible emulation models, we should 
make this as local an option as possible, not as global as possible. 
And given the optimal move emulation model's interaction with legacy 
code (e.g., std::pair), I think the "safe"/"friendly" model should be 
encouraged.

- Something definitive on throwing move constructors.  I feel strangely 
unsatisfied with this so far.  What about Vicente's proposal to use 
has_nothrow_move_constructor and has_nothrow_move_assign rather than 
has_nothrow_move?  What about move_if_noexcept from N2983?  Does boost 
have support for the noexcept keyword?  Failing compiler detection of 
non-throwing move constructors/assignment operators, I think it would be 
a good idea for the move emulation macros to tag classes as non-throwing 
move constructible / non-throwing move assignable, and have the 
corresponding trait, by default, detect this tag.  I know this would 
have problems for recursive containers, but it seems like this is a 
special case, there is a workaround in such cases (partial 
specialization), and it allows users of the emulation macros to 
automatically take advantage of the performance benefits offered by 
detecting non-throwing move construction and assignment.  [This 
paragraph is really just turning into a bunch of random thoughts that 
aren't really organized at all...]

--------

I suggest the following additions could be made to improve the library.

--------

- Techniques to capture (implicitly created) rvalues with emulated 
rvalue references in C++03, e.g., for push_back, when you have knowledge 
of the types of objects you want to capture.  I.e., expound on the 
following technique to capture rvalues of type T, while still allowing 
implicit conversions to T:

template< class U >
typename enable_if< is_convertible< U&, const T& > >::type
f(U& x) { ...copy static_cast< const T& >(x)... }
template< class U >
typename disable_if< is_same< U, T > >::type
f(const U& x) { ...copy static_cast< const T& >(x)... }
void f(rv<T>& x) { ...move x... }

and the following, if you don't need to support conversions to T:

void f(T& x) { ...copy x... }
void f(rv<T> const& x) { ...copy x... }
void f(rv<T>& x) { ...move x... }

- TypeTraits metafunctions is_rvalue_reference, add_rvalue_reference, 
and remove_rvalue_reference.  I'm currently using the following definitions:

template< class T > struct is_rvalue_reference : boost::false_type { };
#ifndef BOOST_NO_RVALUE_REFERENCES
template< class T > struct is_rvalue_reference< T&& > : true_type { };
#else // #ifndef BOOST_NO_RVALUE_REFERENCES
template< class T > struct is_rvalue_reference< rv<T>& > : true_type { };
template< class T > struct is_rvalue_reference< const rv<T>& > : 
true_type { };
#endif // #ifndef BOOST_NO_RVALUE_REFERENCES

#ifndef BOOST_NO_RVALUE_REFERENCES
template< class T > struct add_rvalue_reference { typedef T&& type; };
#else // #ifndef BOOST_NO_RVALUE_REFERENCES
namespace detail_add_rvalue_reference
{
template< class T, bool = is_movable<T>::value >
struct add_rvalue_reference_impl { typedef T type; };
template< class T >
struct add_rvalue_reference_impl< T, true > { typedef rv<T>& type; };
} // namespace detail_add_rvalue_reference
template< class T >
struct add_rvalue_reference
     : detail_add_rvalue_reference::add_rvalue_reference_impl<T>
{ };
#endif // #ifndef BOOST_NO_RVALUE_REFERENCES

template< class T > struct remove_rvalue_reference { typedef T type; };
#ifndef BOOST_NO_RVALUE_REFERENCES
template< class T > struct remove_rvalue_reference< T&& > { typedef T 
type; };
#else // #ifndef BOOST_NO_RVALUE_REFERENCES
template< class T > struct remove_rvalue_reference< rv<T> > { typedef T 
type; };
template< class T > struct remove_rvalue_reference< const rv<T> > { 
typedef T type; };
template< class T > struct remove_rvalue_reference< volatile rv<T> > { 
typedef T type; };
template< class T > struct remove_rvalue_reference< const volatile rv<T> 
 > { typedef T type; };
template< class T > struct remove_rvalue_reference< rv<T>& > { typedef T 
type; };
template< class T > struct remove_rvalue_reference< const rv<T>& > { 
typedef T type; };
template< class T > struct remove_rvalue_reference< volatile rv<T>& > { 
typedef T type; };
template< class T > struct remove_rvalue_reference< const volatile 
rv<T>& > { typedef T type; };
#endif // #ifndef BOOST_NO_RVALUE_REFERENCES

- TypeTraits metafunctions is_lvalue_reference, add_lvalue_reference, 
and remove_lvalue_reference ?  Perhaps add_reference and 
remove_reference can be modified so that they behave wrt emulated rvalue 
references the same as wrt real rvalue references, i.e., add_reference< 
rv<T>& > -> T& rather than rv<T>& (since T&& & -> T&).

- Add'l TypeTraits has_[trivial_]move_{constructor,assign}...?

- An as_lvalue(T& x) function, which amounts to an identity operation in 
C++0x, but strips emulated rvalue references in C++03.  This may be 
necessary to prevent "accidental moves".

- Consider BOOST_RV_REF_TMPL (from a previous email) in addition to / as 
a replacement for BOOST_RV_REF_N_TEMPL_ARGS.

--------

I tried to summarize as much as possible the prior msgs we and others 
had exchanged, but there may be something I left out.  In any case, I 
think this is sufficiently lengthy the way it is... ;)

Let me know what you think,

- Jeff