Fix move.h's to use a concrete RValue carrier object rather than hacking a RValue&.

For move semantics, we need to create a private "RValue" type that is used to
create move constructors and move operators.  Previously, we emulated Boost's
idea of making the RValue type a subclass of the move-only type that doesn't add
any new member fields. We then just reinterpret_cast "this" into a RValue&
depending on the fact that RValue is just a type pun for the move-only type.
This ends up being undefined behavior though (C++98 5.2.10.7).

This change makes use a concrete RValue class that contains a pointer to the
move-only type.  With -O2 on clang version 3.2 (trunk 163674), this yields
identical assembly code to the previous implementation. With -O0, we generate
2 more instructions to allocate and initialize the temporary RValue struct's
object field when calling Pass().

This should be acceptable.  The snowman says so ☃.

BUG=155436

Review URL: https://chromiumcodereview.appspot.com/11078014

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@161945 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 30 insertions, 34 deletions

diff --git a/base/move.h b/base/move.h
index daead8d..7fcb8dd 100644
--- a/base/move.h
+++ b/base/move.h
@@ -69,7 +69,7 @@
 // alternate conversion sequence and a constructor.  We add
 //
 //   * a private struct named "RValue"
-//   * a user-defined conversion "operator RValue&()"
+//   * a user-defined conversion "operator RValue()"
 //   * a "move constructor" and "move operator=" that take the RValue& as
 //     their sole parameter.
 //
@@ -92,8 +92,8 @@
 //
 //     public:
 //       ... API ...
-//       Foo(RValue& other);           // Move constructor.
-//       Foo& operator=(RValue& rhs);  // Move operator=
+//       Foo(RValue other);           // Move constructor.
+//       Foo& operator=(RValue rhs);  // Move operator=
 //    };
 //
 //    Foo MakeFoo();  // Function that returns a Foo.
@@ -111,31 +111,29 @@
 //
 // IMPLEMENTATION SUBTLETIES WITH RValue
 //
-// The RValue struct has subtle properties:
+// The RValue struct is just a container for a pointer back to the original
+// object. It should only ever be created as a temporary, and no external
+// class should ever declare it or use it in a parameter.
 //
-//   1) All its methods are declared, but intentionally not defined.
-//   2) It is *never* instantiated.
-//   3) It is a child of the move-only type.
+// It is tempting to want to use the RValue type in function parameters, but
+// excluding the limited usage here for the move constructor and move
+// operator=, doing so would mean that the function could take both r-values
+// and l-values equially which is unexpected.  See COMPARED To Boost.Move for
+// more details.
 //
-// (1) is a guard against accidental violation of (2).  If an instance of
-// RValue were ever created, either as a temporary, or as a copy to some
-// function parameter or field of a class, the binary will not link.
+// An alternate, and incorrect, implementation of the RValue class used by
+// Boost.Move makes RValue a fieldless child of the move-only type. RValue&
+// is then used in place of RValue in the various operators.  The RValue& is
+// "created" by doing *reinterpret_cast<RValue*>(this).  This has the appeal
+// of never creating a temproary RValue struct even with optimizations
+// disabled.  Also, by virtue of inheritance you can treat the RValue
+// reference as if it were the move-only type itself.  Unfortuantely,
+// using the result of this reinterpret_cast<> is actually undefined behavior
+// due to C++98 5.2.10.7. In certain compilers (eg., NaCl) the optimizer
+// will generate non-working code.
 //
-// This ensures that RValue can only exist as a temporary which is important
-// to avoid accidental dangling references.
-//
-// (3) allows us to get around instantiations because our user-defined
-// conversion can return a downcast of this pointer.
-//
-//    operator RValue&() { return *reinterpret_cast<RValue*>(this); }
-//
-// Because RValue does not extend the object size or add any virtual methods,
-// this type-pun is safe.
-//
-// An alternative implementation would be to make RValue into a concrete
-// struct that holds a reference to the type.  But in the non-optimized build,
-// this causes unnecessary temporaries to be made bloating the object files.
-// Also, it would then be possible to accidentally persist an RValue instance.
+// In optimized builds, both implementations generate the same assembly so we
+// choose the one that adheres to the standard. ☃
 //
 //
 // COMPARED TO C++11
@@ -154,8 +152,8 @@
 //
 // COMPARED TO Boost.Move
 //
-// Our implementation is based on Boost.Move, but we keep the RValue struct
-// private to the move-only type.
+// Our implementation similar to Boost.Move, but we keep the RValue struct
+// private to the move-only type, and we don't use the reinterpret_cast<> hack.
 //
 // In Boost.Move, RValue is the boost::rv<> template.  This type can be used
 // when writing APIs like:
@@ -195,17 +193,15 @@
 //
 #define MOVE_ONLY_TYPE_FOR_CPP_03(type, rvalue_type) \
  private: \
-  struct rvalue_type : public type { \
-    rvalue_type(); \
-    ~rvalue_type(); \
-    rvalue_type(const rvalue_type&); \
-    void operator=(const rvalue_type&); \
+  struct rvalue_type { \
+    rvalue_type(type* object) : object(object) {} \
+    type* object; \
   }; \
   type(type&); \
   void operator=(type&); \
  public: \
-  operator rvalue_type&() { return *reinterpret_cast<rvalue_type*>(this); } \
-  type Pass() { return type(*reinterpret_cast<rvalue_type*>(this)); } \
+  operator rvalue_type() { return rvalue_type(this); } \
+  type Pass() { return type(rvalue_type(this)); } \
  private:
 
 #endif  // BASE_MOVE_H_