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