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-15 22:10:10

On 5/15/2010 4:26 PM, vicente.botet wrote:
> ----- Original Message -----
> From: "OvermindDL1"<overminddl1_at_[hidden]>
> To:<boost-users_at_[hidden]>;<boost_at_[hidden]>;<boost-announce_at_[hidden]>
> Sent: Friday, May 07, 2010 5:36 AM
> Subject: [boost] [Review] Formal Review: Boost.Move
>
>
>> Greetings Boost Developers and Users,
>>
>> It's my pleasure to announce that the review of Ion Gaztañagas' Move
>> library starts May 10th and lasts until May 24th, 2010, unless an
>> extension occurs.
>
> Hi Ion, I have some questions and comments before writting a review.

Vicente, I think I can give an answer to some of your questions, only so 
that you can receive a speedier response.  Of course, Ion is the final 
authority.

> * The use of the is_movable metafunction is not clear in the documentation. The name let think to a concept check, that is, is T a model of the Movable concept? But the check is related to whether T is convertible to rv<T>&, and it seems you uses it in Boost.Container to avoid conflicts using enable_if/disable_if. Could you clarify this? Should is_movable be renamed to a more specific name?
[...]
> * What returns is_movable if BOOST_ENABLE_MOVE_EMULATION is not defined for compilers with rvalue references?

In C++0x, is_movable detects whether a particular nested typedef exists, 
a typedef defined automatically by the move enabling macros.  However, 
as I've mentioned to Ion previously, it looks like is_movable is only 
used in a necessary context within C++03 to detect whether move 
*emulation* is enabled for a type, so it is currently my belief that 
is_movable should be defined *only* for C++03.  I personally find 
is_movable to be a perfectly adequate name for this trait, but perhaps 
its purpose should be more precisely nailed down and explained in the 
documentation.

> * Should the use of BOOST_RV_REF in template classes be protected with is_movable?

I don't think this is necessary as far as using BOOST_RV_REF( T ) as a 
function parameter type.  In C++03, that function will just simply never 
be instantiated if T isn't movable (because a variable of type 
BOOST_RV_REF( T ) would never be created).

> * Could you add the requirements of a Movable type? Could we have a trait for this?

That is also one of the things I think we should consider.

> * I don't know if the next metafunction is correct in C++0x,
>
> // emulation
> template<typename T>
> struct rvalue_ref {
>    typedef ::BOOST_MOVE_NAMESPACE::rv<  T>&   type;
> };
>
> // C++0x
> template<typename T>
> struct rvalue_ref {
>    typedef T&&   type;
> };
>
> If correct, could it be used to replace the macro BOOST_RV_REF?

Because you wouldn't be able to get template parameters to bind to 
function argument types.  Consider

template< class T >
void f(BOOST_RV_REF( T ));

vs

template< class T >
void f(typename rvalue_ref<T>::type);

Other than that, there should be no difference.  Indeed, I think I'm 
going to propose additional metafunctions dealing with rvalue 
references, e.g., add_rvalue_reference, remove_rvalue_reference, 
is_rvalue_reference...

> * BTW, why BOOST_RV_REF_2_TEMPL_ARGS and BOOST_RV_REF_3_TEMPL_ARGS are needed?
>
> #define BOOST_RV_REF(TYPE)\
>     ::BOOST_MOVE_NAMESPACE::rv<  TYPE>&  \
> //
>
> #define BOOST_RV_REF_2_TEMPL_ARGS(TYPE, ARG1, ARG2)\
>     ::BOOST_MOVE_NAMESPACE::rv<  TYPE<ARG1, ARG2>  >&  \
> //
>
> #define BOOST_RV_REF_3_TEMPL_ARGS(TYPE, ARG1, ARG2, ARG3)\
>     ::BOOST_MOVE_NAMESPACE::rv<  TYPE<ARG1, ARG2, ARG3>  >&  \
> //

Consider what the preprocessor would make of BOOST_RV_REF( tmpl< T, U > 
)...oops!  The preprocessor thinks you're passing 2 parameters to 
BOOST_RV_REF :(

Ion, instead of BOOST_RV_REF_N_TEMPL_ARGS, I use something called 
BOOST_RV_REF_TMPL( tmpl, type_seq ) which takes the name of a class 
template tmpl and a Boost.Preprocessor seq type_seq and expands to an 
(emulated) rvalue reference to tmpl< type_seq[0], ..., type_seq[n-1] >:

#define BOOST_RV_REF_TMPL( tmpl, type_seq ) tmpl < BOOST_PP_SEQ_ENUM( 
type_seq ) > &&

or

#define BOOST_RV_REF_TMPL( tmpl, type_seq ) ::boost::rv< tmpl < 
BOOST_PP_SEQ_ENUM( type_seq ) > > &

I prefer using these to BOOST_RV_REF_N_TEMPL_ARGS.  Something to 
consider for addition to / replacement of BOOST_RV_REF_N_TEMPL_ARGS.

> * has_nothrow_move. What are the expected some performance improvements.  of movable classes that specialize this to true? Could an example be added?

I'm hoping more generally to be specific on where we stand with throwing 
move constructors and, if acceptable, how we intend to support them.

If you haven't already, see David Abraham's article at C++Next [1], and 
the link from there to N2983.

> * As the standard uses these more specific ones
> template<class T>  struct has_nothrow_move_constructor;
>
> template<class T>  struct has_nothrow_move_assign;
>
> Shouldn't Boost.Move use these

What are the implementations of these?  Can you give a reference 
confirming they are in the (upcoming) standard?

> * What about the other move traits? Couldn't we let the user specialize them also?
>
> template<class T>  struct has_move_constructor;
>
> template<class T>  struct has_move_assign;
>
> template<class T>  struct has_trivial_move_constructor;
>
> template<class T>  struct has_trivial_move_assign;

I'm unsure whether we should encourage user-specialization for so many 
metafunctions :/

[...]
> Best,
> _____________________
> Vicente Juan Botet Escribá
> http://viboes.blogspot.com/

- Jeff

[1] http://cpp-next.com/archive/2009/10/exceptionally-moving/