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

Subject: Re: [boost] [Review] Formal Review: Boost.Move
From: Thomas Klimpel (Thomas.Klimpel_at_[hidden])
Date: 2010-09-16 19:17:32

Thomas Klimpel wrote:
> The main drawback of the optimized move emulation mode is that it
> forces other classes to also implement an optimized move emulation,
> if they want to derive from an optimized class or use it as member.
> I have the impression that this problem can be solved by introducing a
> suitable macro (let's call it BOOST_MOVE_ASSIGNMENT_WRAPPER),
> that could be defined as follows:
>
> [snip]
>
> As implemented above, constructor forwarding is  still missing,
>  but as boost.move also offers constructor forwarding functionality,
> it should be possible to fix this.

Now that the results for the "constrained value" review finally got announced, I said to myself that I should at least try to finish this wrapper for the optimized move mode. Find attached the updated wrapper and some tests for the wrapper. The tests could still be improved, but after updating the wrapper, I'm no longer sure that the optimized move mode will be needed at all, because

> However, my position on breaking move-assignment from temporaries for the
> non-optimized mode remains that this is not a reasonable option, especially
> considering that it would be a serious regression with respect to the Adobe Move Library.

Perhaps allowing specific movable classes to break move-assignment from temporaries is not too bad after all. Of course, this implies that generic code must write 
generic_movable = boost::move(Create_GenericMovable());
instead of
generic_movable = Create_GenericMovable();
and hence can't benefit from RVO here (and I'm not sure whether the code will still compile when Create_GenericMovable() returns a const reference instead of a value), but will this missed RVO opportunity here be really important? I initially though it was, but I think I was wrong.

I tried to write a simple move enabled class in pure C++0x. In order to see anything related to move semantics at all, the simple class has one pointer member and one "copy and movable" member. Here it is (also attached as "simple_class_c++0x.cpp"):

#include <utility>
class simple_impl {};
class simple_cm {};
class simple_class
{
  typedef simple_class self_t;
  simple_impl* p;
  simple_cm m;
public:
  ~simple_class() { if(p) delete p; }
  simple_class() : p(0), m() {}
  simple_class(const self_t& cr) : p(0), m(cr.m) {
    if(cr.p) p = new simple_impl(*cr.p);
  }
  simple_class(self_t&& rr) : p(rr.p), m(std::move(rr.m)) { rr.p = 0; }

  void swap(self_t&& rr) {
    if (this != &rr) {
      if (p) delete p;
      p = rr.p;
      rr.p = 0;
      m = std::move(rr.m);
    }
  }
  self_t& operator=(self_t&& rr) { swap(std::move(rr)); return *this; }
  self_t& operator=(self_t s) { swap(std::move(s)); return *this; }
};

I was quite disappointed when I found out that the move assignment operator can't be used, because "xxx = std::move(yyy);" just triggers the error message that the assignment operator is ambiguous. So I changed the copy assignment operator from "self_t& operator=(self_t s) { swap(std::move(s)); return *this; }" to "self_t& operator=(const self_t& cr) { swap(self_t(cr)); return *this; }". Now I wondered whether the class has become less efficient because of that change. The efficiency is still the same, because RVO would only kick in for temporaries, and these are now handled just as efficient by the move assignment operator.

But this probably also means that
generic_movable = boost::move(Create_GenericMovable());
won't be less efficient than
generic_movable = Create_GenericMovable();
because RVO wouldn't gain any efficiency here.

I took the freedom to write the assignment operators in terms of "swap" taking its argument by rvalue reference. This "swap" function is actually not my invention, but the new standard offers it for its container classes. It would probably be less confusing to implement a separate function " void move_assign(self_t&& rr)" (identical to the current swap function), and implement swap and the assignment operators in terms of this function:

  void swap(self_t&& rr) { move_assign(std::move(rr)); }
  self_t& operator=(self_t&& rr) { move_assign(std::move(rr)); return *this; }
  //self_t& operator=(self_t s) { move_assign(std::move(s)); return *this; }
  self_t& operator=(const self_t& cr) { move_assign(self_t(cr)); return *this; }

The difference between "move_assign" and the move assignment operator is that "move_assign" doesn't return a reference to "*this", and that it won't render the copy assignment operator ambiguous.

The interesting this about "move_assign" is that even when we implement the assignment operator as taking its argument by value (so that we don't need an assignment operator taking its argument be rvalue reference), we still want to have "move_assign", because it will normally be more efficient than a simple swap.

So one of the non-optimized modes of Boost.Move (the one where the copy assignment operator can't reuse resources) could define

#if !defined(BOOST_NO_RVALUE_REFERENCES)
#define BOOST_COPYABLE_AND_MOVABLE(TYPE)\
...
  TYPE& operator=(const TYPE& cr) { return operator=(TYPE(cr)); }
#else
#define BOOST_COPYABLE_AND_MOVABLE(TYPE)\
...
  TYPE& operator=(TYPE s) { return operator=(boost::move(s)); }
#endif

and the user would just have to supply the definition of

  self_t& operator=(BOOST_RV_REF(self_t) rr)

Another non-optimized modes of Boost.Move (the one currently documented by Boost.Move, where move assignment from temporaries only works with rvalue references) would be to just let the user define
  self_t& operator=(BOOST_RV_REF(self_t) rr)
and
  self_t& operator=(const self_t& cr)
like he would have to do when using pure C++0x.

Hope this clarified things a bit.

Regards,
Thomas