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

Subject: Re: [boost] [Review] Formal Review: Boost.Move
From: Ion Gaztañaga (igaztanaga_at_[hidden])
Date: 2010-05-25 05:26:04

On 25/05/2010 5:21, Thomas Klimpel wrote:
> Many thanks to Ion for his great work and thanks also to OvermindDL1
> for managing this review.

Thanks for your thorough review

> 1) The old library doesn't support "Movable but Non-Copyable" types.
> This makes the old library useless for many important use cases (see
> Boost.Thread for example).

> 2) The old library is not able to exploit true rvalue references for
> compilers that implement them.

> 3) The old library doesn't offer an emulation of std::forward (for
> "Constructor Forwarding").

> 4) The old library implements adobe::move() and automatically move
> assigns from temporaries.

> It seems impossible to achieve (1) or (2) with this design of the
> Adobe Move Library, while the design of Boost.Move up to this point
> doesn't seem to exclude to offer any of the functionality provided by
> the Adobe Move Library. Note that the design up to this point already
> achieved (1). In order to achieve (2), Boost.Move has to define some
> suitable macros, and document their intended usage. It seems to me
> that the existing implementation provides (4), but the existing
> documentation doesn't clearly state this.

I'm sorry, I don't fully understand this. What do you mean by 
"automatically move assigns from temporaries"? That the existing 
implementation allows catching rvalues if the assignment operator 
catches by value?

//RVO constructs c from temporary
copyable &operator=(copyable c)
{  this->swap(c); return *this; }

> So the basic design is sound (I don't expect it to change), but it
> looks as if Boost.Move currently suffers from a proliferation of
> macros. Also the documented conventions how to use the provided
> functionality is probably not yet in its final form.

Is not the best part of the library, I have to admit it ;-)

> With respect to the macros, I think one could try to separate the
> "usability" macros (like BOOST_MOVABLE_BUT_NOT_COPYABLE and
> BOOST_COPYABLE_AND_MOVABLE) from the macros that are required to
> achieve (2).

What do you mean with "separate"? BOOST_MOVABLE_BUT_NOT_COPYABLE & 
BOOST_COPYABLE_AND_MOVABLE ARE "usability" and the rest are required for 
(2)?

>> - What is your evaluation of the implementation?
>
> It's good. There are many examples and tests. And I should add that
> "Constructor Forwarding" is a nice bonus. However, are we sure that
> it is a good idea to call this functionality boost::forward()?  Do we
> know whether it is impossible to implement Boost.Move in standard
> conform C++03 ("strict aliasing rule")?

I've tried some other approaches, but I've failed. Even the promising 
approach explored here:

http://article.gmane.org/gmane.comp.lib.boost.user/58912/match=boost+move

has many downsides because this fails:

void function(movable){}

movable m;

//error: no matching function for call to 'movable::movable(movable)'
//note: candidates are: movable::movable(boost::rv_ref<movable>)
//note:                 movable::movable(movable&)
function(move(m));

>> - What is your evaluation of the documentation?
>
> It's not bad, but it has quite some room for improvement. And
> "two_emulation_modes.html" is simply not acceptable in its current
> form.

Ok. I would need a bit of input on how to explain different options 
offered by the library.

> There is also the question of the role of the documentation for this
> specific library. I don't think that it should say much more about
> the requirements for movable types at the moment. Instead, it should
> try to reference the currently available documentation for this
> topic, for example the "Value Semantics" series
> at<http://cpp-next.com>, but also the most relevant places from the
> upcoming standard.

Ok, but we need a tutorial for newcomers that don't care about language 
issues, just how to make their class movable.

>> - Do you think the library should be accepted as a Boost library?
>
> Yes, but only conditionally on that the documentation in
> "two_emulation_modes.html" gets fixed.

Ok, of course.

> It didn't escape me that Ion wants the reviewers to decide whether
> they prefer the "Optimized mode" or the "Non-optimized mode". In my
> opinion, the limitation of the "Optimized mode" is not acceptable for
> certain classes, especially the containers from Boost.Container. But
> the way the "Non-optimized mode" mode is presented in the
> documentation with the implication that it seems to be unable to
> handle move assignment from temporaries for copyable_and_movable
> would also be hard to accept. However, I believe this is just a
> documentation issue, and it would work if the user would implement
> the assignment operator so that it takes its argument by value.

Both approaches can coexist. We just need to offer another macro for 
BOOST_OPTIMIZED_COPYABLE_AND_MOVABLE. If choice is what boosters 
prefers, I'm open to that, my main goal was to push a move emulation 
that has been blocked for months, if not for years.

Regarding containers, I think (please, correctme if I'm wrong) we can 
only move_assign rvalues in non-optimized mode catching by value. And 
this is a big pessimization for container copy constructors because the 
container does not reuse existing resources:

Container &operator =(Container c)
{
   //c has allocated its own resources.
   //Resources from *this are wasted and destroyed
   //on function exit
   this->swap(c);
}

When catching by reference (in optimized mode) the container can reuse 
all current memory (even objects, if assignment is used like in 
"list::assign" to construct objects) for copy constructors. The 
optimized mode allows maximum efficiency but complicates the assignment 
operator of classes holding containers. But how many classes containing 
containers use trivial assignment operators?

> I wouldn't object to give the users that want to have the extra bit
> of performance the possibility to use the "Optimized mode", but the
> "Non-optimized mode" must stay available, and the macro
> "BOOST_MOVE_OPTIMIZED_EMULATION" should be removed or replaced by
> something less intrusive.

We can offer both approaches with the library, explaining different 
modes of adding move semantics to a class (optimized, non-optimized, 
catch by value, etc.). The problem is what should generic algorithms 
should expect from a class, because for an object in non-optimized mode, 
assignment from a rvalue might not by the best way to implement an 
algorithm, but the algorithm does not know that (and even this has 
consequences with exception guarantees, but this is another issue).

Best,

Ion