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-22 11:00:13

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

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

I see that there is not too much participation on the review of this library. Either people is not interested on Move semantics (I doubt), either they are interested but they have an implementation provided by some compilers in C++0x. This library will allow everybody to play with move semantics without waiting you can use a C++0x compiler in your work, and way is better improving the performances of your applications.

Anyway, here it is my review.

I was waiting for this library since a too long time. Most people think that this is an emulation library, but it is more than that as it implements most of the C++0x Move semantics classes and functions. In addition with the use of the proposed macros the user can write almost portable applications that can rin on c++98 and C++0x compilers.

The fact that Ion has already ported its libraries Container and Intrusive libraries as well as the unordered library let me think that there will be not too much holes. It will be great to have also Interprocess (completly removing the need of the Interprocess specific macors) and  Thread libraries ported :)

How can Boost.Functional/Forward and Boost.Move interact? 

> - What is your evaluation of the design?

This is a little library covering a complex domain. We have no found a better way to emulate move semantics. The limitations are clearly identified and don't add constraints to the common uses. So yes, this is a good design.

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

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

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

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

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

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

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

* How can interact two parts of the same application that uses different optimization mode? Shouldn't be better to define separated macros? Am I missing something evident?

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

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

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

> - What is your evaluation of the documentation?

* The tutorial is well written.
* I find the index not too much structured. I would put together all the tutorials in a Tutorial section.
* I have not found any mention that this is a header only library, maybe I have missed this point.
* I would apreciate if the author states explicitly the dependencies to other Boost libraries.
* An example showing how to make a container or boost::function movable will help to understand a lot of uses.
I will move Containers and move semantics to an Examples section
* 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.
* 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.
* 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.
* A Bibliography section compiling all the referred documents should be nice to have.

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

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

Bravo!

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

> - How much effort did you put into your evaluation? A glance? A quick
> - reading? In-depth study?

A long walk.

> - Are you knowledgeable about the problem domain?

I understand the domain, but unfortunately I'm not an expert in.
 
> - Do you think the library should be accepted as a Boost library?

Yes. Boost must include a common library emmulating move semantics even if there are some known limitations.

Ion, thanks again for all the work done.
_____________________
Vicente Juan Botet Escribá
http://viboes.blogspot.com/