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

Subject: Re: [boost] [Review] Formal Review: Boost.Move
From: vicente.botet (vicente.botet_at_[hidden])
Date: 2010-05-16 06:24:46

----- Original Message ----- 
From: "Jeffrey Lee Hellrung, Jr." <jhellrung_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Sunday, May 16, 2010 4:10 AM
Subject: Re: [boost] [Review] Formal Review: Boost.Move

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

Thanks.
 
>> * 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.

Ion if this is the case, could you change the documentation.

In C++03, is_movable needs to instantiate rv<T> to detect if  T is convertible to T, but for the builting types this should not work as rv<int> generate a compile error. The same applies for enums. Am I missing something? 
 
>> * 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).

You are right for function parameters, but for return types or typedefs we should protect the instantiation, isn"t it? 
 
>> * 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.  

This avoid to use macro and hides the internal implementation.
As builtins don't accept rv instantiation, but copy semantics is equivalent to move semantics

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

Yes, this is interesting. 
 
>> * BTW, why BOOST_RV_REF_2_TEMPL_ARGS and BOOST_RV_REF_3_TEMPL_ARGS are needed?
>>
> 
> 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 :(

Sorry, I have not tried this, but does BOOST_RV_REF(( tmpl< T, U > )) fails?
 
>> * 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.

I did, and will do again, but  I think that the documentation need to explain explicitly or reference here somthing external.

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

The same as has_nothrow_move,, false until specialized explicitly to true.

> Can you give a reference 
> confirming they are in the (upcoming) standard?

N3092 20.7.2 Header <type_traits> synopsis

 
>> * 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 :/

Well, this is the current state of C++0x. I will prefer Boost.Move to be close to C++0x. You, or Boost.Move can always define the more general with the specific.

Best,
Vicente