[boost] [Review][Move] Boost.Move Review Results

Subject: [boost] [Review][Move] Boost.Move Review Results
From: Michael Caisse (boost_at_[hidden])
Date: 2011-02-01 10:22:11

----------------------------
Executive Summary
----------------------------

Ion Gaztañagas' Boost.Move library was reviewed from May 10th, 2010 through
May 24th, 2010. The original review manager was OvermindDL1 who later
went missing. Eleven individuals joined the discussion with the author.
Six reviews were submitted:

            -  5 reviews ACCEPT
            -  1 review  CONDITIONALLY ACCEPT

Ion indicated he would meet the "conditional requirements" which easily
satisfies a community vote of:

               Boost.Move library ACCEPTED

----------------------------
Overview
----------------------------

The Boost.Move library is a highly anticipated addition to the Boost
collection. The primary motivation is to introduce usable move
functionality in "a portable syntax between C++0x and C++03 compilers";
however, as Vicente Botet articulated, "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."

Reviewers' comments:

   - "This library is long overdue!"        [Terry Golubiewski]

   - "The tutorial is well written."        [Vicente Botet]

   - "Compared to my adobe based move implementation, I think
      yours is superior."                   [Daniel James]

   - "There is a lot of ingenuity in making the move emulation work,
      and Ion has made it very accessible and managed to make differences
      between C++03 and C++0x vanish behind macros and free functions."
                                            [Jeff Hellrung]

An active discussion included:

   - exposing boost::rv<T>  publicly
   - The merits of fully defining requirements of boost::rv<T>&
   - Duplicated functionality found in MPL and Type Traits
   - The necessity for two emulation modes
   - The merits of macros
   - Library author usage of Boost.Move
   - Non-throwing move constructors
   - Various potentially useful type traits
   - Other move implementations

While it is unfortunate that a timely review result was not published,
it did provide an opportunity to discuss some of the concerns raised in
the review with actual users of the library.

One such concern was the "two emulation modes." At some point in the
discussions it became classified as "Safe vs. Fast". When coined with
that bias "safe" would be the preference; however, further investigation
and inquiries of actual users showed the "fast" version is perfectly
acceptable as the default. Additionally, "safe" is a misnomer
considering that one mode is not any safer than the other. Each mode
has benefits and limitations as enumerated by Ion in the documentation
and review responses. The past six months of use would indicate that
defaulting to the "fast" version, as preferred by Ion, is in practice a
good decision.

Thank you Ion for this well thought out and excellent submittal.

Thank you reviewers for expending time and energy to further Boost.
Without those who are dedicated to review submitted libraries, the entire
process and Boost itself falls apart.

The community will benefit from the effort each of you put into this
library.

Best regards -
Michael Caisse

----------------------------
Summary of changes
----------------------------

The following changes are planned based on discussions from the review:

-- Conditional Acceptance --

1. "Two Emulation Mode" documentation typos must be resolved. The
    descriptions, requirements and limitations need to be perfectly
    clear for the end user.

-- Other Issues --

1. The class rv should be public and documented.

2. is_movable should be renamed and the documentation clarified. It should
    only be enabled in C++03. A possible alternative name is
    has_move_emulation_enabled.

3. Nearly all reviewers voiced a preference that the move library should
    use existing functionality from MPL and TypeTraits libraries. While
    on one hand this creates a dependency between move and other libraries
    it results in reuse of accepted and well tested libraries. If a
    non-coupling option is also provided, it should not be the default.

4. Additional traits discussed. Resolution: add to TypeTraits library as
    actual use-cases are found.

5. Provide a boost/move.hpp that includes boost/move/move.hpp

6. Document the interaction of the two emulation modes.

7. Include example for making a container "movable".

8. Two emulation modes - Ion was hopeful that reviewers would have a
    uniform opinion that would result in a single mode. The trade-offs
    identified by Ion in the documentation were apparent to all reviewers;
    however, different conclusions were drawn and consensus was not met.
    The granularity of control was a common issue with nearly all reviewers.
    Therefore, keep both emulation modes with Optimized as the default and
    the granularity of control at or more "local level" than the
    macro provides (class level).

9. emulation_limitations.html: s/overaloading/overloading/

10. Fully qualify enable_if ( ::boost::emable_if )

11. Optimized version of forward should use identity so it will not
     try to deduce T.

12. Make sure required headers are mentioned for tutorials.

13. is_class in is_movable

     template<  class T>
     struct is_movable
         : mpl::and_<  is_class<T>,
                      mpl::not_<  is_rvalue_reference<T>  >,
                      is_convertible<  T, rv<T>&  >  >
     {};

     - or -

     another solution to is_movable problems with references.

14. On throwing move constructors, Ion should follow as close to the
     standard's behaviour as possible. This can be revisited after the
     initial release since it is a performance optimization.

-- 
Michael Caisse
Object Modeling Designs
www.objectmodelingdesigns.com