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..7af15da045d8f10ffd1419f0289b123e213fb79d 100644
--- a/base/memory/scoped_ptr.h
+++ b/base/memory/scoped_ptr.h
@@ -94,6 +94,7 @@
 #include <assert.h>
 #include <stddef.h>
 #include <stdlib.h>
+#include <algorithm> // TODO(ajwong): Do we really want std::swap?

[Jeffrey Yasskin, 2012/10/13 01:33:29]

Yeah, we need to include std::swap in the overload sets. C++11 says we can
#include <utililty>, which is lighter, but C++98 doesn't permit that. You might
try it anyway to see if our libraries let us.

[awong, 2012/10/18 02:46:15]

Fixed locally. Will be up in next patch.

 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
@@ -107,6 +108,37 @@ 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 C>
+struct DefaultDeleter {
+  inline void operator()(C* ptr) const {
+    enum { type_must_be_complete = sizeof(C) };
+    delete ptr;
+  }
+};
+
+// Specialization of DefaultDeleter for array types.
+template <class C>
+struct DefaultDeleter<C[]> {
+  inline void operator()(C* ptr) const {
+    enum { type_must_be_complete = sizeof(C) };
+    delete[] ptr;
+  }
+};
+
+// 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 +149,72 @@ template <typename T> struct IsNotRefCounted {
   };
 };
 
+// Minimal implementation of the core logic of scoped_ptr, suitable for
+// reuse in both scoped_ptr and its specialization.
+template <class C, class D>

[Jeffrey Yasskin, 2012/10/13 01:33:29]

I'd try for better names than "C" and "D".

[awong, 2012/10/18 02:46:15]

Done.

+class scoped_ptr_impl {
+ public:
+  explicit scoped_ptr_impl(C* p) : data_(p) { }
+
+  ~scoped_ptr_impl() {
+    if (data_.ptr != NULL) {
+      (static_cast<D&>(data_))(data_.ptr);

[Jeffrey Yasskin, 2012/10/13 01:33:29]

Give data_ a .deleter() method? Strictly, you could just call
data_(data_.ptr()), but that's confusing.

[awong, 2012/10/18 02:46:15]

Done.

+    }
+  }
+
+  void reset(C* p) {
+    // This self-reset check is deprecated.
+    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. See http://b/6987235.

[Jeffrey Yasskin, 2012/10/13 01:33:29]

Clean up b/ references before committing.

[awong, 2012/10/18 02:46:15]

Removed.  Still need to file a bug, but first I want more confidence that this
unification can even go in.

+        (static_cast<D&>(data_))(data_.ptr);
+      }
+      data_.ptr = p;
+    }
+  }
+
+  C* get() const { return data_.ptr; }
+
+  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);
+  }
+
+  C* release() {
+    C* retVal = data_.ptr;
+    data_.ptr = NULL;
+    return retVal;
+  }
+
+ private:
+  // 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(C* ptr_in) : ptr(ptr_in) {}
+
+    C* ptr;
+  };
+
+  Data data_;
+
+  // Disallow copy and assignment.
+  scoped_ptr_impl(const scoped_ptr_impl&);
+  scoped_ptr_impl& operator=(const scoped_ptr_impl&);
+};
+
+
 }  // namespace internal
+
 }  // namespace base
 
 // A scoped_ptr<T> is like a T*, except that the destructor of scoped_ptr<T>
@@ -129,7 +226,7 @@ template <typename T> struct IsNotRefCounted {
 //
 // The size of a scoped_ptr is small:
 // sizeof(scoped_ptr<C>) == sizeof(C*)
-template <class C>
+template <class C, class D = base::DefaultDeleter<C> >
 class scoped_ptr {
   MOVE_ONLY_TYPE_FOR_CPP_03(scoped_ptr, RValue)
 
@@ -140,27 +237,26 @@ class scoped_ptr {
 
   // The element type
   typedef C 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) { }
+  explicit scoped_ptr(C* p = NULL) : impl_(p) { }
 
   // Constructor.  Allows construction from a scoped_ptr rvalue for a
   // convertible type.
-  template <typename U>
-  scoped_ptr(scoped_ptr<U> other) : ptr_(other.release()) { }
+  //
+  // TODO(ajwong): Correctly handle conversion with different deleters which
+  // can happen on an upcast.  What I think needs to happen is that we need to
+  // be able to converst V to D.
+  template <typename U, typename V>
+  scoped_ptr(scoped_ptr<U, V> other) : impl_(NULL) { //: impl_(other.release()) { 
+  }
 
   // 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_;
+      : impl_(rvalue.object->release()) {
   }
 
   // operator=.  Allows assignment from a scoped_ptr rvalue for a convertible
@@ -179,43 +275,50 @@ class scoped_ptr {
 
   // Reset.  Deletes the current owned object, if any.
   // Then takes ownership of a new object, if given.
-  // this->reset(this->get()) works.
+  // this->reset(this->get()) works, but this behavior is DEPRECATED, and
+  // will be removed
+  //
+  // TODO(ajwong): File bug of it.
   void reset(C* p = NULL) {
+    impl_.reset(p);
+    /*
     if (p != ptr_) {
-      enum { type_must_be_complete = sizeof(C) };
-      delete ptr_;
+      C* old_ptr = ptr_;
       ptr_ = p;
+      // TODO(ajwong): This changes the delete ordering. Verify it's okay.
+      if (old_ptr != NULL) {
+        D()(old_ptr);
+      }
     }
+    */
   }
 
   // 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_;
+    assert(impl_.get() != NULL);
+    return *impl_.get();
   }
   C* operator->() const  {
-    assert(ptr_ != NULL);
-    return ptr_;
+    assert(impl_.get() != NULL);
+    return impl_.get();
   }
-  C* get() const { return ptr_; }
+  C* get() const { return impl_.get(); }
 
   // Allow scoped_ptr<C> 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; }
+  operator Testable() const { return impl_.get() ? &impl_.get() : 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==(C* p) const { return impl_.get() == p; }
+  bool operator!=(C* 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.
@@ -224,18 +327,16 @@ class scoped_ptr {
   // 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;
+    return impl_.release();
   }
 
-  template <typename PassAsType>
-  scoped_ptr<PassAsType> PassAs() {
-    return scoped_ptr<PassAsType>(release());
+  template <typename PassAsType, typename DeleteAsType = base::DefaultDeleter<PassAsType> >
+  scoped_ptr<PassAsType, DeleteAsType> PassAs() {
+    return scoped_ptr<PassAsType, DeleteAsType>(release());
   }
 
  private:
-  C* ptr_;
+  base::internal::scoped_ptr_impl<C, D> impl_;
 
   // 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
@@ -246,18 +347,18 @@ class scoped_ptr {
 };
 
 // Free functions
-template <class C>
-void swap(scoped_ptr<C>& p1, scoped_ptr<C>& p2) {
+template <class C, class D>
+void swap(scoped_ptr<C, D>& p1, scoped_ptr<C, D>& p2) {
   p1.swap(p2);
 }
 
-template <class C>
-bool operator==(C* p1, const scoped_ptr<C>& p2) {
+template <class C, class D>
+bool operator==(C* p1, const scoped_ptr<C, D>& p2) {
   return p1 == p2.get();
 }
 
-template <class C>
-bool operator!=(C* p1, const scoped_ptr<C>& p2) {
+template <class C, class D>
+bool operator!=(C* p1, const scoped_ptr<C, D>& p2) {
   return p1 != p2.get();
 }