Add a move constructor and move assignment operator to scoped_ptr.

base/memory/scoped_ptr.h

Index: base/memory/scoped_ptr.h
diff --git a/base/memory/scoped_ptr.h b/base/memory/scoped_ptr.h
index 869bdfdbe92f6d8ab1f086c4b05afd9f1b7d207a..b68f0e015482c355e46389717801f18477028ffa 100644
--- a/base/memory/scoped_ptr.h
+++ b/base/memory/scoped_ptr.h
@@ -86,6 +86,7 @@
 
 #include <iosfwd>
 #include <memory>
+#include <type_traits>
 #include <utility>
 
 #include "base/basictypes.h"
@@ -258,35 +259,76 @@ class scoped_ptr {
   // Constructor.  Allows construction from a nullptr.
   scoped_ptr(std::nullptr_t) : impl_(nullptr) {}
 
-  // Constructor.  Allows construction from a scoped_ptr rvalue for a
+  // Move constructor.
+  //
+  // IMPLEMENTATION NOTE: Clang requires a move constructor to be defined (and
+  // not just the conversion constructor) in order to warn on pessimizing moves.
+  // The requirements for the move constructor are specified in C++11
+  // 20.7.1.2.1.15-17, which has some subtleties around reference deleters. As
+  // we

[vmpstr, 2015/11/19 21:00:24]

<_<

[danakj, 2015/11/19 21:03:36]

wrapping broke

[dcheng, 2015/11/19 21:26:04]

Oops.

+  // don't support reference (or move-only) deleters, the post conditions are
+  // trivially true: we always copy construct the deleter from other's deleter.
+  scoped_ptr(scoped_ptr&& other) : impl_(&other.impl_) {}
+
+  // Conversion constructor.  Allows construction from a scoped_ptr rvalue for a
   // convertible type and deleter.
   //
-  // IMPLEMENTATION NOTE: C++11 unique_ptr<> keeps this constructor distinct
-  // from the normal move constructor. By C++11 20.7.1.2.1.21, this constructor
-  // has different post-conditions if D is a reference type. Since this
-  // implementation does not support deleters with reference type,
-  // we do not need a separate move constructor allowing us to avoid one
-  // use of SFINAE. You only need to care about this if you modify the
-  // implementation of scoped_ptr.
-  template <typename U, typename V>
-  scoped_ptr(scoped_ptr<U, V>&& other)
-      : impl_(&other.impl_) {
-    COMPILE_ASSERT(!base::is_array<U>::value, U_cannot_be_an_array);
+  // IMPLEMENTATION NOTE: C++ 20.7.1.2.1.19 requires this constructor to only
+  // participate in overload resolution if all the following are true:
+  // - U is implicitly convertible to T: this is important for 2 reasons:
+  //     1. So type traits don't incorrectly return true, e.g.
+  //          std::is_convertible<scoped_ptr<Base>, scoped_ptr<Derived>>::value
+  //        should be false.
+  //     2. To make sure code like this compiles:
+  //        void F(scoped_ptr<int>);
+  //        void F(scoped_ptr<Base>);
+  //        // ambiguous since both conversion constructors match.

[danakj, 2015/11/19 21:03:36]

nit: capital A

[dcheng, 2015/11/19 21:26:04]

Done.

+  //        F(scoped_ptr<Derived>());
+  // - U is not an array type: to prevent conversions from scoped_ptr<T[]> to
+  //   scoped_ptr<T>.
+  // - D is a reference type and E is the same type, or D is not a reference
+  //   type and E is implicitly convertible to D: again, we don't support
+  //   reference deleters, so we only worry about the latter requirement.
+  template <typename U,
+            typename E,

[dcheng, 2015/11/19 20:49:10]

I renamed the second template parameter from V to E, since it matches more
logically with D -> E.

[danakj, 2015/11/19 21:03:36]

sgtm

+            typename = typename std::enable_if<
+                !std::is_array<U>::value &&
+                std::is_convertible<typename scoped_ptr<U, E>::pointer_type,
+                                    pointer_type>::value &&
+                std::is_convertible<E, D>::value>::type>
+  scoped_ptr(scoped_ptr<U, E>&& other)
+      : impl_(&other.impl_) {}
+
+  // operator=.
+  //
+  // IMPLEMENTATION NOTE: Unlike the move constructor, Clang does not appear to
+  // require a move assignment operator to trigger the pessimizing move warning:
+  // in this case, the warning triggers when moving a temporary. For consistency
+  // with the move constructor, we define it anyway. C++11 20.7.1.2.3.1-3
+  // defines several requirements around this: like the move constructor, the
+  // requirements are simplified by the fact that we don't support move-only or
+  // reference deleters.
+  scoped_ptr& operator=(scoped_ptr&& rhs) {
+    impl_.TakeState(&rhs.impl_);
+    return *this;
   }
 
   // operator=.  Allows assignment from a scoped_ptr rvalue for a convertible
   // type and deleter.
   //
   // IMPLEMENTATION NOTE: C++11 unique_ptr<> keeps this operator= distinct from
-  // the normal move assignment operator. By C++11 20.7.1.2.3.4, this templated
-  // form has different requirements on for move-only Deleters. Since this
-  // implementation does not support move-only Deleters, we do not need a
-  // separate move assignment operator allowing us to avoid one use of SFINAE.
-  // You only need to care about this if you modify the implementation of
-  // scoped_ptr.
-  template <typename U, typename V>
-  scoped_ptr& operator=(scoped_ptr<U, V>&& rhs) {
-    COMPILE_ASSERT(!base::is_array<U>::value, U_cannot_be_an_array);
+  // the normal move assignment operator. C++11 20.7.1.2.3.4-7 contains the
+  // requirement for this operator, but like the conversion constructor, the
+  // requirements are greatly simplified by not supporting move-only or
+  // reference deleters.
+  template <typename U,
+            typename E,
+            typename = typename std::enable_if<

[vmpstr, 2015/11/19 21:00:24]

So, to elaborate on what I was saying earlier, you can bypass this enable_if by
explicitly specifying the third template argument. The reason for this is that
it's a default value that has the enable if, but if you specify the type the
default isn't used.

One of the ways to fix it is something along the lines of:

template <typename U,
          typename E,
          typename std::enable_if<..., int>::type = 0>

which makes the enable_if be the actual type of the third parameter (int), which
means you can't bypass it even if you specify the third template parameter.

The other option of course is what the standard library does with a private type
and a second parameter to the ctor.

[dcheng, 2015/11/19 21:26:04]

This is the only way I could get it to compile:
template <typename U,
          typename E,
          typename std::enable_if<...>::type* = nullptr>

[vmpstr, 2015/11/19 21:28:41]

My guess is you didn't put the ", int" part in the enable_if :) But void* works
as well as anything else. Thanks.

+                !std::is_array<U>::value &&
+                std::is_convertible<typename scoped_ptr<U, E>::pointer_type,
+                                    pointer_type>::value &&
+                std::is_convertible<E, D>::value>::type>
+  scoped_ptr& operator=(scoped_ptr<U, E>&& rhs) {
     impl_.TakeState(&rhs.impl_);
     return *this;
   }
@@ -362,8 +404,9 @@ class scoped_ptr<T[], D> {
 
  public:
   // The element and deleter types.
-  typedef T element_type;
-  typedef D deleter_type;
+  using element_type = T;
+  using pointer_type = element_type*;

[dcheng, 2015/11/19 20:49:10]

This is for consistency, but probably not needed, strictly speaking.

[vmpstr, 2015/11/19 21:00:24]

FWIW, I think reading

std::is_convertible<U*, T*>::value

is a lot easier :)

[danakj, 2015/11/19 21:03:36]

unique_ptr has ::pointer, not pointer_type. and it has it on both the
unique_ptr<T> and unique_ptr<T[]>.

https://github.com/llvm-mirror/libcxx/blob/master/include/memory#L2562
https://github.com/llvm-mirror/libcxx/blob/master/include/memory#L2748

[dcheng, 2015/11/19 21:26:04]

Given vmpstr@'s feedback and the fact that our pointer type alias isn't quite
the same as unique_ptr, I've just removed it.

+  using deleter_type = D;
 
   // Constructor.  Defaults to initializing with nullptr.
   scoped_ptr() : impl_(nullptr) {}