Re: [boost] [move][container] Review Request (new versions of Boost.Move and Boost.Container in sandbox and vault)

Subject: Re: [boost] [move][container] Review Request (new versions of Boost.Move and Boost.Container in sandbox and vault)
From: Ion Gaztañaga (igaztanaga_at_[hidden])
Date: 2009-08-24 02:57:51

Jeffrey Hellrung escribió:
> Ion Gaztañaga wrote:
>> ...new versions of libraries Boost.Move and Boost.Container.
> 
> Few comments on proposed Boost.Move:

Thanks for the comments!

> - Make all metafunctions true MPL metafunctions (as mentioned in 
> previous postings).

Ok, but I would like to avoid any header dependency with MPL so that 
library is minimal.

> - BOOST_COPYABLE_AND_MOVABLE definition scopes relative to the global 
> scope, while other macros seems to scope relative to the local scope 
> (e.g., the difference between ::boost::rv<TYPE> and boost::rv<TYPE>). 
> I'm a stickler for consistency, and would probably consider 
> ::boost::rv<TYPE> et al to be safer.

Yes, thanks.

> - What is the rationale for placing BOOST_COPYABLE_AND_MOVABLE and 
> BOOST_MOVABLE_BUT_NOT_COPYABLE in the private section of a class 
> definition?  IIRC, BOOST_ENABLE_MOVE_EMULATION was suppose to go in the 
> public section, and I'm generally wondering what the motivations are for 
> specifying public or private.

I just wanted to maintain public part as clean as possible, but there is 
no strong argument for that.

> - Even though this can be gleaned from the code, I'd like to see a "How 
> it works" section, e.g., how exactly does the library coerce rvalues 
> into binding to the BOOST_RV_REF overload of operator= rather than the 
> BOOST_COPY_ASSIGN_REF overload?  Perhaps more generally, I think some 
> rationale for why this particular emulation strategy was chosen over 
> others would be nice (e.g., the previous proposed Boost.Move used a 
> by-value operator= to assign from rvalues efficiently; why the change? 
> How does this compare to Adobe's move emulation?).  In most other 
> respects, I think the documentation is very good and complete.

Ok, I'll add this.

> - I would've renamed BOOST_COPY_REF_N_TEMPL_ARGS to 
> BOOST_COPY_ASSIGN_REF_N_TEMPL_ARGS just to be consistent with 
> BOOST_RV_REF and BOOST_RV_REF_N_TEMPL_ARGS.

Yes, thanks.

> - Can you comment at all on the stability of the names of the "class 
> declaration" macros (e.g., BOOST_MOVABLE_BUT_NOT_COPYABLE)?  I recall a 
> previous post that suggested alternative names.  I have no opinion 
> either way, I was just curious on the status of that suggestion.

I don't have any name preference, this can be discussed in the review so 
that we find a suitable name.

> - In the clone_ptr example, would it be more correct to check for 
> self-assignment in the move assignment operator (as is done in the copy 
> assignment operator) rather than not?

Ok.

> - Overall, great job.  If there's anything I can do to help the 
> transition, let me know.
> 
> - Jeff

Thanks!

Ion