Extend scoped_ptr to be closer to unique_ptr. Support custom deleters, and deleting arrays.

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 3547b7a15397f9c58f3c5193401d6d9defae333d..bbe200cd2d7a0fde04eb31574e85d69c1a43b272 100644
--- a/base/memory/scoped_ptr.h
+++ b/base/memory/scoped_ptr.h
@@ -95,6 +95,8 @@
 #include <stddef.h>
 #include <stdlib.h>
 
+#include <algorithm>  // For std::swap().
+
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/move.h"
@@ -107,6 +109,51 @@ class RefCountedBase;
 class RefCountedThreadSafeBase;
 }  // namespace subtle
 
+// Function object which deletes its parameter, which must be a pointer.
+// If C is an array type, invokes 'delete[]' on the parameter; otherwise,
+// invokes 'delete'. The default deleter for scoped_ptr<T>.
+template <class T>
+struct DefaultDeleter {
+  DefaultDeleter() {}
+  template <typename U> DefaultDeleter(const DefaultDeleter<U>& other) {
+    // All default single-object deleters can trivially convert to one another.
+  }
+  inline void operator()(T* ptr) const {
+    enum { type_must_be_complete = sizeof(T) };
+    delete ptr;
+  }
+};
+
+// Specialization of DefaultDeleter for array types.
+template <class T>
+struct DefaultDeleter<T[]> {
+  inline void operator()(T* ptr) const {
+    enum { type_must_be_complete = sizeof(T) };
+    delete[] ptr;
+  }
+
+ private:
+  // Disable this operator for any U != T because it is undefined to execute
+  // an array delete when the static type of the array mismatches the dynamic
+  // type.
+  //
+  // References:
+  //   C++98 [expr.delete]p3
+  //   http://cplusplus.github.com/LWG/lwg-defects.html#938
+  template <typename U> void operator()(U* array) const;
+};
+
+// Function object which invokes 'free' on its parameter, which must be
+// a pointer. Can be used to store malloc-allocated pointers in scoped_ptr:
+//
+// scoped_ptr<int, base::FreeDeleter> foo_ptr(
+//     static_cast<int*>(malloc(sizeof(int))));
+struct FreeDeleter {
+  inline void operator()(void* ptr) const {
+    free(ptr);
+  }
+};
+
 namespace internal {
 
 template <typename T> struct IsNotRefCounted {
@@ -117,7 +164,104 @@ template <typename T> struct IsNotRefCounted {
   };
 };
 
+// Minimal implementation of the core logic of scoped_ptr, suitable for
+// reuse in both scoped_ptr and its specializations.
+template <class T, class D>
+class scoped_ptr_impl {

[Ryan Sleevi, 2012/11/29 02:42:27]

Have you confirmed what the compile time implications are of having this common
template interface?

I realize the desire to favour readability, but it seems like you're forcing
potentially four template instantiations during resolution. That is, for any
given T, it needs to see scoped_ptr<T,D>, which requires instantiation
scoped_ptr_impl<T,D>, then see if the specialization of scoped_ptr<T[],D> fits,
which requires instantiation of scoped_ptr_impl<T[],D>. At least, AIUI the
current resolution processes in clang and MSVC.

It seems that not having a common base, as admittedly harmful as that may be to
readability, and just having the two overloads (T and T[]), combined with the
two deleters (D<V> and D<V[]>), may make a slight-in-isolation-but-large-overall
perf impact.

May be micro-optimization, but given that tricks like SFINAE-ing the Pass()
implementation for Callback() caused a 3% MSVC regression, or the extra arg for
base::Bind was something like a 16% regression, I want to make sure we're not
going too crazy.

[awong, 2012/12/12 02:17:03]

I'm not quite sure how to go about testing this.  Do you just run time on winja?

My gut tells me this will not nearly be as expensive. Bind() is a bit degenerate
because of the N^2 template specializations. However, that's gut feel without
data.

[Ryan Sleevi, 2012/12/12 03:37:47]

Usually just 3-5 timed clobber builds, run by script, on the major platforms we
care about - once without change and once with - and see what (if any)
regression in the mean.

[awong, 2012/12/12 21:00:14]

Grrr...can I just test on the build bots? :D

(I don't actually have working builds on any platform than Linux)

+ public:
+  explicit scoped_ptr_impl(T* p) : data_(p) { }
+
+  // Initializer for deleters that have data parameters.
+  scoped_ptr_impl(T* p, const D& d) : data_(p, d) {}
+
+  // Templated constructor that destructively takes the value from another
+  // scoped_ptr_impl.
+  template <typename U, typename V>
+  scoped_ptr_impl(scoped_ptr_impl<U, V>* other)
+      : data_(other->release(), other->get_deleter()) {
+    // We do not support move-only deleters.  We could modify our move
+    // emulation to have base::subtle::move() and base::subtle::forward()
+    // functions that are imperfect emulations of their C++11 equivalents,
+    // but until there's a requirement, just assume deleters are copyable.
+  }
+
+  template <typename U, typename V>
+  void TakeState(scoped_ptr_impl<U, V>* other) {
+    // See comment in templated constructor above regarding lack of support
+    // for move-only deleters.
+    reset(other->release());
+    get_deleter() = other->get_deleter();
+  }
+
+  ~scoped_ptr_impl() {
+    if (data_.ptr != NULL) {
+      // Not using get_deleter() saves one function call in non-optimized
+      // builds.
+      static_cast<D&>(data_)(data_.ptr);
+    }
+  }
+
+  void reset(T* p) {
+    // This self-reset check is deprecated.
+    // this->reset(this->get()) currently works, but it is DEPRECATED, and
+    // will be removed once we verify that no one depends on it.
+    //
+    // TODO(ajwong): Change this behavior to match unique_ptr<>.
+    // http://crbug.com/162971
+    if (p != data_.ptr) {
+      if (data_.ptr != NULL) {
+        // Note that this can lead to undefined behavior and memory leaks
+        // in the unlikely but possible case that get_deleter()(get())
+        // indirectly deletes this. The fix is to reset ptr_ before deleting
+        // its old value, but first we need to clean up the code that relies
+        // on the current sequencing.

[gromer, 2012/12/04 19:41:18]

Maybe add a tracking bug for this too?

[awong, 2012/12/12 02:17:03]

I think http://crbug.com/162971 cited earlier covers this, or are you seeing
something else?

[Ryan Sleevi, 2012/12/12 03:37:47]

Ah, I just fail at reading comprehension then.

+        static_cast<D&>(data_)(data_.ptr);
+      }
+      data_.ptr = p;
+    }
+  }
+
+  T* get() const { return data_.ptr; }
+
+  D& get_deleter() { return data_; }
+  const D& get_deleter() const { return data_; }
+
+  void swap(scoped_ptr_impl& p2) {
+    // Standard swap idiom: 'using std::swap' ensures that std::swap is
+    // present in the overload set, but we call swap unqualified so that
+    // any more-specific overloads can be used, if available.
+    using std::swap;
+    swap(static_cast<D&>(data_), static_cast<D&>(p2.data_));
+    swap(data_.ptr, p2.data_.ptr);
+  }
+
+  T* release() {
+    T* old_ptr = data_.ptr;
+    data_.ptr = NULL;
+    return old_ptr;
+  }
+
+ private:
+  // Needed to allow type-converting constructor.
+  template <typename U, typename V> friend class scoped_ptr_impl;
+
+  // Use the empty base class optimization to allow us to have a D
+  // member, while avoiding any space overhead for it when D is an
+  // empty class.  See e.g. http://www.cantrip.org/emptyopt.html for a good
+  // discussion of this technique.
+  struct Data : public D {
+    explicit Data(T* ptr_in) : ptr(ptr_in) {}
+    Data(T* ptr_in, const D& other) : D(other), ptr(ptr_in) {}
+    T* ptr;
+  };
+
+  Data data_;
+
+  DISALLOW_COPY_AND_ASSIGN(scoped_ptr_impl);
+};
+
 }  // namespace internal
+
 }  // namespace base
 
 // A scoped_ptr<T> is like a T*, except that the destructor of scoped_ptr<T>
@@ -129,93 +273,81 @@ template <typename T> struct IsNotRefCounted {
 //
 // The size of a scoped_ptr is small:
 // sizeof(scoped_ptr<C>) == sizeof(C*)
-template <class C>
+template <class T, class D = base::DefaultDeleter<T> >
 class scoped_ptr {
   MOVE_ONLY_TYPE_FOR_CPP_03(scoped_ptr, RValue)
 
-  COMPILE_ASSERT(base::internal::IsNotRefCounted<C>::value,
-                 C_is_refcounted_type_and_needs_scoped_refptr);
+  COMPILE_ASSERT(base::internal::IsNotRefCounted<T>::value,
+                 T_is_refcounted_type_and_needs_scoped_refptr);
 
  public:
-
-  // The element type
-  typedef C element_type;
+  // The element and deleter types.
+  typedef T element_type;
+  typedef D deleter_type;
 
   // Constructor.  Defaults to initializing with NULL.
-  // There is no way to create an uninitialized scoped_ptr.
-  // The input parameter must be allocated with new.
-  explicit scoped_ptr(C* p = NULL) : ptr_(p) { }
+  scoped_ptr() : impl_(NULL) { }
+
+  // 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 scoped_ptr rvalue for a
-  // convertible type.
-  template <typename U>
-  scoped_ptr(scoped_ptr<U> other) : ptr_(other.release()) { }
+  // convertible type and deleter.
+  template <typename U, typename V>
+  scoped_ptr(scoped_ptr<U, V> other) : impl_(&other.impl_) { }
 
   // Constructor.  Move constructor for C++03 move emulation of this type.
-  scoped_ptr(RValue rvalue)
-      : ptr_(rvalue.object->release()) {
-  }
-
-  // Destructor.  If there is a C object, delete it.
-  // We don't need to test ptr_ == NULL because C++ does that for us.
-  ~scoped_ptr() {
-    enum { type_must_be_complete = sizeof(C) };
-    delete ptr_;
-  }
+  scoped_ptr(RValue rvalue) : impl_(&rvalue.object->impl_) { }
 
   // operator=.  Allows assignment from a scoped_ptr rvalue for a convertible
-  // type.
-  template <typename U>
-  scoped_ptr& operator=(scoped_ptr<U> rhs) {
-    reset(rhs.release());
+  // type and deleter.
+  template <typename U, typename V>
+  scoped_ptr& operator=(scoped_ptr<U, V> rhs) {
+    impl_.TakeState(&rhs.impl_);
     return *this;
   }
 
-  // operator=.  Move operator= for C++03 move emulation of this type.
-  scoped_ptr& operator=(RValue rhs) {
-    swap(*rhs->object);
-    return *this;
-  }
-
-  // Reset.  Deletes the current owned object, if any.
+  // Reset.  Deletes the currently owned object, if any.
   // Then takes ownership of a new object, if given.
-  // this->reset(this->get()) works.
-  void reset(C* p = NULL) {
-    if (p != ptr_) {
-      enum { type_must_be_complete = sizeof(C) };
-      delete ptr_;
-      ptr_ = p;
-    }
-  }
+  void reset(element_type* p = NULL) { impl_.reset(p); }
 
   // Accessors to get the owned object.
   // operator* and operator-> will assert() if there is no current object.
-  C& operator*() const {
-    assert(ptr_ != NULL);
-    return *ptr_;
+  element_type& operator*() const {
+    assert(impl_.get() != NULL);
+    return *impl_.get();
   }
-  C* operator->() const  {
-    assert(ptr_ != NULL);
-    return ptr_;
+  element_type* operator->() const  {
+    assert(impl_.get() != NULL);
+    return impl_.get();
   }
-  C* get() const { return ptr_; }
+  element_type* get() const { return impl_.get(); }
 
-  // Allow scoped_ptr<C> to be used in boolean expressions, but not
+  // Access to the deleter.
+  deleter_type& get_deleter() { return impl_.get_deleter(); }
+  const deleter_type& get_deleter() const { return impl_.get_deleter(); }
+
+  // Allow scoped_ptr<element_type> to be used in boolean expressions, but not
   // implicitly convertible to a real bool (which is dangerous).
-  typedef C* scoped_ptr::*Testable;
-  operator Testable() const { return ptr_ ? &scoped_ptr::ptr_ : NULL; }
+ private:

[gromer, 2012/12/04 19:41:18]

This violates the style guide by interleaving private with public, and by
interleaving type declarations with function declarations. Is there any other
way?

[awong, 2012/12/12 02:17:03]

Yeah...I could move the typedef into a private section, but it makes the idiom
harder to read IMO. Will ask a few other readability reviewers on chromium for
their opinion.

+  typedef base::internal::scoped_ptr_impl<element_type, deleter_type>
+      scoped_ptr::*Testable;
+
+ public:
+  operator Testable() const { return impl_.get() ? &scoped_ptr::impl_ : NULL; }
 
   // Comparison operators.
   // These return whether two scoped_ptr refer to the same object, not just to
   // two different but equal objects.
-  bool operator==(C* p) const { return ptr_ == p; }
-  bool operator!=(C* p) const { return ptr_ != p; }
+  bool operator==(element_type* p) const { return impl_.get() == p; }
+  bool operator!=(element_type* p) const { return impl_.get() != p; }
 
   // Swap two scoped pointers.
   void swap(scoped_ptr& p2) {
-    C* tmp = ptr_;
-    ptr_ = p2.ptr_;
-    p2.ptr_ = tmp;
+    impl_.swap(p2.impl_);
   }
 
   // Release a pointer.
@@ -223,44 +355,163 @@ class scoped_ptr {
   // If this object holds a NULL pointer, the return value is NULL.
   // After this operation, this object will hold a NULL pointer,
   // and will not own the object any more.
-  C* release() WARN_UNUSED_RESULT {
-    C* retVal = ptr_;
-    ptr_ = NULL;
-    return retVal;
+  element_type* release() WARN_UNUSED_RESULT {
+    return impl_.release();
   }
 
+  // C++98 doesn't support functions templates with default parameters which
+  // makes it hard to write a PassAs() that understands converting the deleter
+  // while preserving simple calling semantics.
+  //
+  // Until there is a use case for PassAs() with custom deleters, just ignore
+  // the custom deleter.
   template <typename PassAsType>
   scoped_ptr<PassAsType> PassAs() {
-    return scoped_ptr<PassAsType>(release());
+    return scoped_ptr<PassAsType>(Pass());
   }
 
  private:
-  C* ptr_;
+  // Needed to reach into |impl_| in the constructor.
+  template <typename U, typename V> friend class scoped_ptr;
+  base::internal::scoped_ptr_impl<element_type, deleter_type> impl_;
+
+  // Forbid comparison of scoped_ptr types.  If U != T, it totally
+  // doesn't make sense, and if U == T, it still doesn't make sense
+  // because you should never have the same object owned by two different
+  // scoped_ptrs.
+  template <class U> bool operator==(scoped_ptr<U> const& p2) const;
+  template <class U> bool operator!=(scoped_ptr<U> const& p2) const;
+};
+
+template <class T, class D>
+class scoped_ptr<T[], D> {
+  MOVE_ONLY_TYPE_FOR_CPP_03(scoped_ptr, RValue)
+
+ public:
+  // The element and deleter types.
+  typedef T element_type;
+  typedef D deleter_type;
+
+  // Constructor.  Defaults to initializing with NULL.
+  scoped_ptr() : impl_(NULL) { }
+
+  // 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
+  //   incorrectly calculated). Deletion is also always undefined
+  //   (C++98 [expr.delete]p3). If you're doing this, fix your code.
+  // - it cannot be NULL, because NULL is an integral expression, not a
+  //   pointer to T. Use the no-argument version instead of explicitly
+  //   passing NULL.
+  // - it cannot be const-qualified differently from T per unique_ptr spec
+  //   (http://cplusplus.github.com/LWG/lwg-active.html#2118). Users wanting
+  //   to work around this may use the implicit_cast<const T*>() idiom.
+  //   However, because of the first bullet in this comment, users MUST
+  //   NOT use implicit_cast<Base*>() to upcast the static type of the array.
+  explicit scoped_ptr(element_type* array) : impl_(array) { }
+
+  // Constructor.  Move constructor for C++03 move emulation of this type.
+  scoped_ptr(RValue rvalue) : impl_(&rvalue.object->impl_) { }
+
+  // operator=.  Move operator= for C++03 move emulation of this type.
+  scoped_ptr& operator=(RValue rhs) {
+    impl_.TakeState(&rhs.object->impl_);
+    return *this;
+  }
+
+  // Reset.  Deletes the currently owned array, if any.
+  // Then takes ownership of a new object, if given.
+  void reset(element_type* array = NULL) { impl_.reset(array); }
+
+  // Accessors to get the owned array.
+  element_type& operator[](size_t i) const {
+    assert(impl_.get() != NULL);
+    return impl_.get()[i];
+  }
+  element_type* get() const { return impl_.get(); }
+
+  // Access to the deleter.
+  deleter_type& get_deleter() { return impl_.get_deleter(); }
+  const deleter_type& get_deleter() const { return impl_.get_deleter(); }
+
+  // Allow scoped_ptr<element_type> to be used in boolean expressions, but not
+  // implicitly convertible to a real bool (which is dangerous).
+ private:
+  typedef base::internal::scoped_ptr_impl<element_type, deleter_type>
+      scoped_ptr::*Testable;
+
+ public:
+  operator Testable() const { return impl_.get() ? &scoped_ptr::impl_ : NULL; }
+
+  // Comparison operators.
+  // These return whether two scoped_ptr refer to the same object, not just to
+  // two different but equal objects.
+  bool operator==(element_type* array) const { return impl_.get() == array; }
+  bool operator!=(element_type* array) const { return impl_.get() != array; }
+
+  // Swap two scoped pointers.
+  void swap(scoped_ptr& p2) {
+    impl_.swap(p2.impl_);
+  }
+
+  // Release a pointer.
+  // The return value is the current pointer held by this object.
+  // If this object holds a NULL pointer, the return value is NULL.
+  // After this operation, this object will hold a NULL pointer,
+  // and will not own the object any more.
+  element_type* release() WARN_UNUSED_RESULT {
+    return impl_.release();
+  }
 
-  // Forbid comparison of scoped_ptr types.  If C2 != C, it totally doesn't
-  // make sense, and if C2 == C, it still doesn't make sense because you should
-  // never have the same object owned by two different scoped_ptrs.
-  template <class C2> bool operator==(scoped_ptr<C2> const& p2) const;
-  template <class C2> bool operator!=(scoped_ptr<C2> const& p2) const;
+ private:
+  // Force element_type to be a complete type.
+  enum { type_must_be_complete = sizeof(element_type) };
 
+  // Actually hold the data.
+  base::internal::scoped_ptr_impl<element_type, deleter_type> impl_;
+
+  // Disable initialization from any type other than element_type*, by
+  // providing a constructor that matches such an initialization, but is
+  // private and has no definition. This is disabled because it is not safe to
+  // call delete[] on an array whose static type does not match its dynamic
+  // type.
+  template <typename U>
+  explicit scoped_ptr(U* array);
+
+  // Disable reset() from any type other than element_type*, for the same
+  // reasons as the constructor above.
+  template <typename U>
+  void reset(U* array);
+
+  // Forbid comparison of scoped_ptr types.  If U != T, it totally
+  // doesn't make sense, and if U == T, it still doesn't make sense
+  // because you should never have the same object owned by two different
+  // scoped_ptrs.
+  template <class U> bool operator==(scoped_ptr<U> const& p2) const;
+  template <class U> bool operator!=(scoped_ptr<U> const& p2) const;
 };
 
 // Free functions
-template <class C>
-void swap(scoped_ptr<C>& p1, scoped_ptr<C>& p2) {
+template <class T, class D>
+void swap(scoped_ptr<T, D>& p1, scoped_ptr<T, D>& p2) {
   p1.swap(p2);
 }
 
-template <class C>
-bool operator==(C* p1, const scoped_ptr<C>& p2) {
+template <class T, class D>
+bool operator==(T* p1, const scoped_ptr<T, D>& p2) {
   return p1 == p2.get();
 }
 
-template <class C>
-bool operator!=(C* p1, const scoped_ptr<C>& p2) {
+template <class T, class D>
+bool operator!=(T* p1, const scoped_ptr<T, D>& p2) {
   return p1 != p2.get();
 }
 
+// DEPRECATED: Use scoped_ptr<C[]> instead.
+//
 // scoped_array<C> is like scoped_ptr<C>, except that the caller must allocate
 // with new [] and the destructor deletes objects with delete [].
 //
@@ -298,7 +549,7 @@ class scoped_array {
 
   // operator=.  Move operator= for C++03 move emulation of this type.
   scoped_array& operator=(RValue rhs) {
-    swap(*rhs.object);
+    reset(rhs.object->release());
     return *this;
   }
 
@@ -380,19 +631,12 @@ bool operator!=(C* p1, const scoped_array<C>& p2) {
   return p1 != p2.get();
 }
 
-// This class wraps the c library function free() in a class that can be
-// passed as a template argument to scoped_ptr_malloc below.
-class ScopedPtrMallocFree {
- public:
-  inline void operator()(void* x) const {
-    free(x);
-  }
-};
-
+// DEPRECATED: Use scoped_ptr<C, base::FreeDeleter> instead.
+//
 // scoped_ptr_malloc<> is similar to scoped_ptr<>, but it accepts a
 // second template argument, the functor used to free the object.
 
-template<class C, class FreeProc = ScopedPtrMallocFree>
+template<class C, class FreeProc = base::FreeDeleter>
 class scoped_ptr_malloc {
   MOVE_ONLY_TYPE_FOR_CPP_03(scoped_ptr_malloc, RValue)
 
@@ -420,7 +664,7 @@ class scoped_ptr_malloc {
 
   // operator=.  Move operator= for C++03 move emulation of this type.
   scoped_ptr_malloc& operator=(RValue rhs) {
-    swap(*rhs.object);
+    reset(rhs.object->release());
     return *this;
   }
 
@@ -429,8 +673,10 @@ class scoped_ptr_malloc {
   // this->reset(this->get()) works.
   void reset(C* p = NULL) {
     if (ptr_ != p) {
-      FreeProc free_proc;
-      free_proc(ptr_);
+      if (ptr_ != NULL) {
+        FreeProc free_proc;
+        free_proc(ptr_);
+      }
       ptr_ = p;
     }
   }