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-22 14:56:04

On 22/05/2010 17:00, vicente.botet wrote:

> * The class rv should be public and documented, so people prefering
> to don't use macros can work with the library.

No problem if that's what users prefer.

> * As pointed in another post is_movable should be renamed and its
> intent clarified (and maybe constrained to C++98 mode) with some
> examples, maybe show some parts of the implementation of a movable
> container.

Yes, is_movable is useful but the name is not very useful, even less 
according to the C++0x "movable" concept which includes copy 
constructible types, that is, anything that can be constructed from a 
rvalue.

> * has_nothrow_move. Clarify on the documentation the expected
> performance improvements of movable classes that specialize this to
> true, and add an example on the tutorial.

Ok.

> * I think that, as the standard, the library should work with more
> specific traits than has_nothrow_move, as each operation could have
> its own specificities. template<class T>  struct
> has_nothrow_move_constructor; template<class T>  struct
> has_nothrow_move_assign;

Ok. By default, I guess those should be false, unless we have compiler 
intrinsics around.

> * I guess the standard traits can not be defined other than by the
> compiler, but if no the traits could be specialized by the user, the
> library will provide just the prototype. 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;

Yes, this could be an option. For c++0x we have "noexcept()".

> The library can add these on the TypeTraits if this is the prefered
> place.

This is upt o TypeTraits maintainers. We can start experimenting in 
Boost.Move, but if in the future we move them to TypeTraits (which seems 
to be the correct place, we will need to maintain compatibility with 
those). Maybe move_type_traits.hpp could be the header containing those 
and in the future this could just include apropriate TypeTraits headers.

> * I will add a file boost/move.hpp that include the file
> boost/move/move.hpp

Ok.

> * Why  the macros BOOST_MOVABLE_BUT_NOT_COPYABLE and
> BOOST_COPYABLE_AND_MOVABLE must be included in the private part. For
> me this is part of the public interface.

I have no preference for this, it was originally in the public part but 
in previous pre-review comments some preferred in the private part since 
considered it an "implementation detail".

>> - What is your evaluation of the implementation?
>
> * The implementation is complex as emmulating this language feature
> is not simple. It will be great if the section "How it works?"
> explained how forward works on emmulated mode.

Ok.

> * I don't like the use of macros to handle with issues Interprocess
> porting. Guess this is temporary until Interprocess will migrate to
> use Boost.Move (Or is there a particular issue?). * Implementation
> details as metafunctions identity and is_convertible should be reused
> from mpl and type-traits instead of redefining them.

Yes, it's temporary, it was just to maintain a single source for two 
libraries, because maitenance was becoming a pain. no problem for 
identity and is_convertible.

>> - What is your evaluation of the documentation?

> * I find the index not too much structured. I would put together all the tutorials in a Tutorial section.

What do you mean by "tutorial"?

> * I have not found any mention that this is a header only library, maybe I have missed this point.

Ok.

> * I would apreciate if the author states explicitly the dependencies to other Boost libraries.

Ok.

> * An example showing how to make a container or boost::function movable will help to understand a lot of uses.

Ok. A container would be easier for me.

> * The reference section should include a complete detail of the description of each function, its effects, returned values possible exceptions, ... Currently there are too much functions for which the prototype is the single documentation.

Ok.

> * I would like that the macros reference description states clearly what is behind the scenes, as the user needs to clearly understand what operations are defined and which one s/he doesn't needs to define.

Ok

> * A comparaison with the wrapper way should be welcome as well as a brief history of when move semantics was implemented the first time and how this has evolved over time.

Ok, I consider this somewhat secondary, but I will definitely add it 
once other suggestions are added to the library

> * A Bibliography section compiling all the referred documents should be nice to have.

Ok.

> * Duplicated line in
> two_emulation_modes.html#move.two_emulation_modes.optimized_mode
> copyable_and_movable cm;

Ok

>> - What is your evaluation of the potential usefulness of the
>> library? - Did you try to use the library? With what compiler? Did
>> you have any problems?
>
> Yes, cygwin gcc-3.4, mingw-4.4 msvc Express9
>
> I have run the tests on Boost.Move and Boost.Containers (C++98 mode)
> with no problem with cygwin gcc-3.4, mingw-4.4 and msvc Express9.
>
> Just some really minor warning cygwin gcc-3.4
> move_iterator.cpp:104:3: warning: no newline at end of file
> vector_test.cpp:174:3: warning: no newline at end of file

Ok

>
> Bravo!
>
> I have some run-time errors with mingw-4.4, but I suspect that my
> installation is not as stable as I would.

I think this is a strict aliasing issue, arised in Interprocess, because 
we are accesing T from a type ::boost::rv<T> that does not exist. Adding 
a alias attribute seems to fix the problem. See:

https://svn.boost.org/trac/boost/ticket/3950

> Ion, thanks again for all the work done. _________________

Thanks for the review

Ion