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

Subject: Re: [boost] [Review] Formal Review: Boost.Move
From: Thomas Klimpel (Thomas.Klimpel_at_[hidden])
Date: 2010-05-24 23:21:46

OvermindDL1 wrote:
> It's my pleasure to announce that the review of Ion Gaztañagas' Move
> library starts today, May 10th and lasts until May 24th, 2010, unless
> an extension occurs.

Many thanks to Ion for his great work and thanks also to OvermindDL1 for managing this review.

>  - What is your evaluation of the design?

Compared to the Adobe Move Library, Boost.Move seems to be much more complicated. (I'm referring to the version from ASL 1.0.42 here. Earlier versions like that from ASL 1.0.34 are a different story. So if my opinion surprises you, this might explain it.) So we have to ask what it offers over the old library that could justify this added complexity.

1) The old library doesn't support "Movable but Non-Copyable" types. This makes the old library useless for many important use cases (see Boost.Thread for example).
2) The old library is not able to exploit true rvalue references for compilers that implement them.
3) The old library doesn't offer an emulation of std::forward (for "Constructor Forwarding").
4) The old library implements adobe::move() and automatically move assigns from temporaries.

While (2) and (3) are nice to have, they are less important than (1) and (4). But since (2) is one of the design goals of Boost.Move, it is also very important.

The essence of the design of the Adobe Move Library is

template <typename T>
struct move_from
{
    explicit move_from(T& x) : source(x) { }
    T& source;
};
template <typename T>
T move(T& x, typename move_sink<T>::type = 0) { return T(move_from<T>(x)); }
template <typename T>
T& move(T& x, typename copy_sink<T>::type = 0) { return x; }

and the essence of the design of Boost.Move is

namespace boost {
template <class T>
class rv : public T
{
   rv();
   ~rv();
   rv(rv const&);
   void operator=(rv const&);
};
}
#define XXX_MOVABLE_XXX(TYPE) ... \
   operator ::boost::rv<TYPE>&() ... \
   operator const ::boost::rv<TYPE>&() const ...\

These conversion operators will kick in when no other better matching overload is available. The trick here is that temporaries can't match against "TYPE&", so as long as no "const TYPE&" or "TYPE" overload is available, the conversion operators will kick in.

It seems impossible to achieve (1) or (2) with this design of the Adobe Move Library, while the design of Boost.Move up to this point doesn't seem to exclude to offer any of the functionality provided by the Adobe Move Library. Note that the design up to this point already achieved (1). In order to achieve (2), Boost.Move has to define some suitable macros, and document their intended usage. It seems to me that the existing implementation provides (4), but the existing documentation doesn't clearly state this.

So the basic design is sound (I don't expect it to change), but it looks as if Boost.Move currently suffers from a proliferation of macros. Also the documented conventions how to use the provided functionality is probably not yet in its final form.

With respect to the macros, I think one could try to separate the "usability" macros (like BOOST_MOVABLE_BUT_NOT_COPYABLE and BOOST_COPYABLE_AND_MOVABLE) from the macros that are required to achieve (2).

>  - What is your evaluation of the implementation?

It's good. There are many examples and tests. And I should add that "Constructor Forwarding" is a nice bonus. However, are we sure that it is a good idea to call this functionality boost::forward()?  Do we know whether it is impossible to implement Boost.Move in standard conform C++03 ("strict aliasing rule")?

Also, I think that the definition of BOOST_COPYABLE_AND_MOVABLE and BOOST_MOVABLE_BUT_NOT_COPYABLE is correct in that it ends with "private:". This enables the user to put it at the top of the class declaration, and have the least surprise in case he puts it somewhere else (if he forgets the "public:" after it, the worst that can happen is a compile error).

>  - What is your evaluation of the documentation?

It's not bad, but it has quite some room for improvement. And "two_emulation_modes.html" is simply not acceptable in its current form. It starts with things like "holder& operator(holder &)", then continues with things like Daniel James wrote:
> I think there's a typo in
> 'libs/move/doc/html/move/two_emulation_modes.html', under 'optimized
> mode' you write 'needs to define a copy constructor for
> copyable_and_movable' but the code is an assignment operator.

and becomes unacceptable when it states "cm = copyable_and_movable(); //temporary object is COPIED", without suggesting a proper workaround (which would be to simply follow the approach of the Adobe Move Library for copyable_and_movable classes), not to mention explaining the possible trade-offs that the user can choose from.

There is also the question of the role of the documentation for this specific library. I don't think that it should say much more about the requirements for movable types at the moment. Instead, it should try to reference the currently available documentation for this topic, for example the "Value Semantics" series at <http://cpp-next.com>, but also the most relevant places from the upcoming standard.

>  - What is your evaluation of the potential usefulness of the library?

Extremely useful, as we all agree :)

>  - Did you try to use the library?

Some time ago, I played around with an older version of it:
<http://lists.boost.org/Archives/boost/2009/09/156034.php>

>    With what compiler? Did you have any problems?

gcc 4.3.2 and MSVC-9.0express
No problems. I noticed that templates are the domain were the most important differences to true rvalue semantics show up. But Boost.Move is nevertheless well suited for templates, as the examples and Boost.Container show.

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

A week about six month ago and two days now.

>  - Are you knowledgeable about the problem domain?

I use "swap" since a long time to work around the missing move semantics in places where it is important. I have reviewed quite some documentation about move semantics (including the Adobe Move Library) since I tried to review Boost.Move six month ago.

> And finally, every review should answer this question:
> 
>  - Do you think the library should be accepted as a Boost library?

Yes, but only conditionally on that the documentation in "two_emulation_modes.html" gets fixed.

I don't think that anything in the core implementation needs to be changed, and I also have no problems with the current organization of the macros. However, I think that some changes to the macros will occur when "BOOST_MOVE_OPTIMIZED_EMULATION" gets removed (as planed by Ion).

It didn't escape me that Ion wants the reviewers to decide whether they prefer the "Optimized mode" or the "Non-optimized mode". In my opinion, the limitation of the "Optimized mode" is not acceptable for certain classes, especially the containers from Boost.Container. But the way the "Non-optimized mode" mode is presented in the documentation with the implication that it seems to be unable to handle move assignment from temporaries for copyable_and_movable would also be hard to accept. However, I believe this is just a documentation issue, and it would work if the user would implement the assignment operator so that it takes its argument by value.

I wouldn't object to give the users that want to have the extra bit of performance the possibility to use the "Optimized mode", but the "Non-optimized mode" must stay available, and the macro "BOOST_MOVE_OPTIMIZED_EMULATION" should be removed or replaced by something less intrusive.

Regards,
Thomas