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

Subject: Re: [boost] [Review] Formal Review: Boost.Move
From: Terry Golubiewski (tjgolubi_at_[hidden])
Date: 2010-05-22 16:14:54

> - What is your evaluation of the design?

The design is great, bringing the best of several good designs together and 
supports C++2003 and C++0X.

> - What is your evaluation of the implementation?  Very good.
> - What is your evaluation of the documentation?   Didn't read it (yet).
> - What is your evaluation of the potential usefulness of the library?

This library is long overdue!
I have been using move emulation for several years.  There are several 
things that you just can't do well without move semantics.
Also, boost has already several different methods of move emulation that 
should be united using this library.
Move semantics do significantly improve program performance without much 
extra programmer effort, especially from users.
I only use a modified version of move.hpp and haven't used any of the 
move-aware containers yet.

> - Did you try to use the library? With what compiler? Did you have any 
> problems?

The modified move.hpp that I use is attached.
I have been using a revised (see below and attached) version of this library 
for several months.
I have implemented a version of Howard Hinnants unique_ptr<> emulation using 
it.
I have successfully run Howard Hinnant's unique_ptr<> test suite using it.
I use move() for message passing, resource management (ala Alexandrescu's 
LockingPtr and InternallyLocked), and C++0x thread emulation.
I don't have/use move-aware containers yet.
I had a few problems given in my notes below.

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

 In-depth study of move.hpp

> - Are you knowledgeable about the problem domain?

Yes.

> - Do you think the library should be accepted as a Boost library?

Yes!!! (emphatically)

* Having extra '{' in "//namespace *{" comments makes my editor (vim) 
unhappy.

* class rv: I needed the destructor to be public to compile
  for Microsoft VC2008.

* for Gnu C++, I added __attribute__((__may_alias__)) to rv<>.

* I needed a slightly different version of is_convertible for some reason 
that
  I've forgotton now. This version was mostly taken from Howard Hinnant's
  unique_ptr<> emulation.  Then I hacked it until it worked for Microsoft
  Visual C++ 2008 and GCC 4.3.

  namespace move_detail {

  namespace is_conv_impl {

  typedef char one;
  struct two { char _[2]; };
  template<typename T> one test1(const T&);
  template<typename T> two test1(...);
  template<typename T> one test2(T);
  template<typename T> two test2(...);
  template<typename T> T source();

  } // is_conv_impl

  template<class Fm, class To, bool = true>
  struct is_convertible_helper: public boost::is_convertible<Fm, To> { };

  template<class Fm, class To>
  struct is_convertible_test1: public boost::mpl::bool_<
    sizeof(is_conv_impl::test1<To>(is_conv_impl::source<Fm>())) == 1>
  { };

  template<class Fm, class To>
  struct is_convertible_test2: public boost::mpl::bool_<
    sizeof(is_conv_impl::test2<To>(is_conv_impl::source<Fm>())) == 1>
  { };

#if defined(__GNUG__)
  template<class Fm, class To>
  struct is_convertible_helper<Fm, To, true>
    : public is_convertible_test1<Fm, To> { };

  // For move-only types
  template<class T>
  struct is_convertible_helper<T, T, true>
    : public is_convertible_test2<T, T> { };

#elif defined(_MSCVER)

  template <class Fm, class To>
  struct is_convertible_helper<Fm, To, true>
    : public is_convertible_test2<Fm, To> { };

#endif

  template<class Fm, class To>
  struct is_convertible: public is_convertible_helper<Fm, To> { };

  } // move_detail

* Added is_class<T> check to is_movable

  template<class T>
  struct is_movable
    : public boost::mpl::and_< boost::is_class<T>
                            , move_detail::is_convertible< T, rv<T>& >
                            >
  { };

* Yet another boost::identity<T>?

* Add support for:

  is_rvalue_reference<>
  is_lvalue_reference<>
  remove_reference<>
  etc.

* Add "inline" to move()'s and forward()'s.

* Add additional move():

  template<class T> inline
  typename boost::disable_if< is_movable<T>, const T& >::type
  move(const T& x) { return x; }

* My forward()'s are different.  I remember changing them because of a 
change
* in the C++0X standard.

  template<class T, class U> inline
  typename boost::enable_if_c<
      is_lvalue_reference<T>::value
    && move_detail::is_convertible< U*
                                  , typename remove_reference<T>::type*
                                  >::value
  , T >::type
  forward(U& u) { return static_cast<T>(u); }

  template<class T, class U> inline
  typename boost::enable_if_c<
      !is_reference<T>::value
    && move_detail::is_convertible< U*, T* >::value
  , typename add_rvalue_reference<T>::type >::type
  forward(U& u) { return move(u); }

  template<class T, class U> inline
  typename boost::enable_if_c<
      !is_reference<T>::value
    && move_detail::is_convertible< U* , T* >::value
  , const T& >::type
  forward(const U& u) { return static_cast<const T&>(u); }

  template<class T, class U> inline
  typename boost::enable_if_c<
      !is_reference<T>::value
    && move_detail::is_convertible< U* , T* >::value
  , typename add_rvalue_reference<T>::type >::type
  forward(rv<U>& u) { return move(static_cast<T&>(static_cast<U&>(u))); }

* I have a swap defined like this.  Perhaps boost::swap should be updated if
* the move library is accepted.

  template<class T1, class T2> inline
  void swap(T1& a, T2& b) {
    T1 tmp(std0x::move(a));
    a = std0x::move(b);
    b = std0x::move(tmp);
  } // swap

* In class move_iterator:
    operator++(int),
    operator--(int),
    operator+(difference_type n) const
    operator-(difference_type n) const
  have extra <iterator_type>.

* Added "inline" to make_move_iterator().
    make_move_iterator()
    uninitialized_move()
    uninitialized_move_move_iterator()
    uninitialized_copy_or_move()