Re: [Boost-users] [Review] Formal Review: Boost.Move

Subject: Re: [Boost-users] [Review] Formal Review: Boost.Move
From: Neil Groves (neil_at_[hidden])
Date: 2010-05-23 17:55:38

2010/5/23 Ion Gaztañaga <igaztanaga_at_[hidden]>

> On 23/05/2010 21:09, Steven Watanabe wrote:
>
>> I'm wondering whether it would be better to
>> use something like
>>
>> template<class T>
>> struct rv {
>> T* impl;
>> operator const T&() const;
>> };
>>
>> template<class T>
>> T& unwrap_rv(rv<T>&);
>>
>> I know the interface isn't quite as nice, and
>> there will be more cases where overload resolution
>> needs a hand, but it seems safer.
>>
>
I might be being a bit slow, but doesn't replacing the inheritance of rv<T>
from T with the contained pointer change the lifetime and ownership of the T
object in problematic ways?

Anyhow here is my review:
1. What is your evaluation of the implementation?
In general it is a good well executed modern C++ implementation. There are a
few compiler portability issues such as the is_convertible implementation
and the accessability of the boost::rv destructor.

I have found the macro idiom to work very well and it does not obfuscate
code. I like the macros since they are able to guarantee a completely
optimal upgrade path to proper rvalue references where these are supported
by the compiler.

For me the most serious implementation issue is the non-compliant aliasing
when move emulation is selected. For the forseeable future most of the
compilers that we support will use the emulation support. While the tested
behaviour on many compilers has not shown defects, it is my opinion that
this is not an acceptable approach for Boost. We must not have core
libraries that depend on undefined behaviour. While I appreciate that GCC
4.4 has been the first to show defect symptoms due to the aliasing, it is
quite conceivable that problems would arise after compiler revisions or
patches that we would not be able to fix. This is especially important
considering the huge usefulness of this library.

2. What is your evaluation of the documentation?

There are a couple of small spelling mistakes that have already been pointed
out on the list, but I found the documentation entirely adequate to use the
library confidently.

3. What is your evaluation of the potential usefulness of the library?

Clearly Boost.Move will be enormously useful in client code and as a tool
for other Boost libraries. There are plenty of benchmarks that show
substantial performance benefits with rvalue references, and from my timings
a good emulation achieves very similar results. I am very keen to introduce
move support into Boost.Range.

4. Did you try to use the library? With what compiler? Did you have any
problems?

I tried to use the library with my own unit tests, and integrated into a
large-scale application. I used Intel C++ 10.0, 10.1, 11.0, GCC 4.3, 4.4.3,
MSVC 8, MSVC 9, MSVC10.

I did have a few problems. The rv destructor needs to be public for VC.

The I had to replace the is_convertible implementation with the one from
type_traits.

The violating of the strict aliasing rules was causing problems with
correctness on GCC 4.4, and I tried a couple of modifications:

The first modification was to replace the reinterpret_casts with
static_casts.

The second modification was to add the __attribute__ may_alias to the GCC
4.4 move emulation. This has the very strange effect of causing all of my
unit tests to pass, but causes subsequent compilation failures in regions of
code that use the move support. The failure appears to be that a function of
the movable class cannot be found, and the compiler lists the available
functions, one of which is exactly the function signature being looked for!
Hence I believe that may_alias replaces on set of problems for another less
obvious set of problems.

5. How much effort did you put into your evaluation?

I have put in approximately 40 hours of evaluation and experimentation with
the compiler set mentioned. I placed much of my time examining the generated
code for performance issues, and exploring solutions to the aliasing rule
violation.

6. Are you knowledgeable about the problem domain?

I am familiar with the advantages of move support, and the relevant language
specifications.

7. Do you think the library should be accepted as a Boost library.

Yes, if and only if a fully compliant move emulation mechanism is produced.
Absence of evidence of error on compilers is not sufficient in my opinion.

I would gladly accept a more complex interface for a defined implementation.

Thank you Ion for putting this together, and please let me know if I can
assist in any way.

Regards,
Neil Groves