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