Add a move constructor and move assignment operator to scoped_ptr.

base/memory/scoped_ptr.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Scopers help you manage ownership of a pointer, helping you easily manage a
 // pointer within a scope, and automatically destroying the pointer at the end
 // of a scope.  There are two main classes you will use, which correspond to the
 // operators new/delete and new[]/delete[].
 //
 // Example usage (scoped_ptr<T>):
@@ skipping 68 matching lines @@
 
 // This is an implementation designed to match the anticipated future TR2
 // implementation of the scoped_ptr class.
 
 #include <assert.h>
 #include <stddef.h>
 #include <stdlib.h>
 
 #include <iosfwd>
 #include <memory>
+#include <type_traits>
 #include <utility>
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/move.h"
 #include "base/template_util.h"
 
 namespace base {
 
 namespace subtle {
@@ skipping 152 matching lines @@
 
   // Constructor.  Takes ownership of p.
   explicit scoped_ptr(element_type* p) : impl_(p) {}
 
   // Constructor.  Allows initialization of a stateful deleter.
   scoped_ptr(element_type* p, const D& d) : impl_(p, d) {}
 
   // 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;
   }
 
   // operator=.  Allows assignment from a nullptr. Deletes the currently owned
   // object, if any.
   scoped_ptr& operator=(std::nullptr_t) {
     reset();
     return *this;
   }
@@ skipping 55 matching lines @@
   // Forbidden for API compatibility with std::unique_ptr.
   explicit scoped_ptr(int disallow_construction_from_null);
 };
 
 template <class T, class D>
 class scoped_ptr<T[], D> {
   MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03(scoped_ptr)
 
  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) {}
 
   // Constructor. Stores the given array. Note that the argument's type
   // must exactly match T*. In particular:
   // - it cannot be a pointer to a type derived from T, because it is
   //   inherently unsafe in the general case to access an array through a
   //   pointer whose dynamic type does not match its static type (eg., if
   //   T and the derived types had different sizes access would be
@@ skipping 174 matching lines @@
 scoped_ptr<T> make_scoped_ptr(T* ptr) {
   return scoped_ptr<T>(ptr);
 }
 
 template <typename T>
 std::ostream& operator<<(std::ostream& out, const scoped_ptr<T>& p) {
   return out << p.get();
 }
 
 #endif  // BASE_MEMORY_SCOPED_PTR_H_