Replace 'operator*' with explicit 'get' method on SmartPointer

src/smart-pointers.h

Index: src/smart-pointers.h
diff --git a/src/smart-pointers.h b/src/smart-pointers.h
index 7c35b2aff27196a65d4e5e3e30326d33c292ad1f..1a3f178e9b1735bb853cfeed5cd887cb2b546246 100644
--- a/src/smart-pointers.h
+++ b/src/smart-pointers.h
@@ -36,14 +36,14 @@ template<typename Deallocator, typename T>
 class SmartPointerBase {
  public:
   // Default constructor. Constructs an empty scoped pointer.
-  inline SmartPointerBase() : p_(NULL) {}
+  SmartPointerBase() : p_(NULL) {}
 
   // Constructs a scoped pointer from a plain one.
-  explicit inline SmartPointerBase(T* ptr) : p_(ptr) {}
+  explicit SmartPointerBase(T* ptr) : p_(ptr) {}
 
   // Copy constructor removes the pointer from the original to avoid double
   // freeing.
-  inline SmartPointerBase(const SmartPointerBase<Deallocator, T>& rhs)
+  SmartPointerBase(const SmartPointerBase<Deallocator, T>& rhs)
       : p_(rhs.p_) {
     const_cast<SmartPointerBase<Deallocator, T>&>(rhs).p_ = NULL;
   }
@@ -51,20 +51,21 @@ class SmartPointerBase {
   // When the destructor of the scoped pointer is executed the plain pointer
   // is deleted using DeleteArray.  This implies that you must allocate with
   // NewArray.
-  inline ~SmartPointerBase() { if (p_) Deallocator::Delete(p_); }
+  ~SmartPointerBase() { if (p_) Deallocator::Delete(p_); }

[svenpanne, 2013/12/03 16:17:55]

Hmmm, this has been like that before, but why is it safe to have a non-virtual
destructor in a base class here? As it is, nothing prevents an incorrect usage
AFAICT...

[yurys, 2013/12/04 07:34:01]

This is a good question. I believe the problem is that SmartArrayPointer and
SmartPointer inherit from SmartPointerBase. This could be solved e.g. by making
SmartPointer a template with single parameter T. Then it may use Deallocator as
type traits internally, Deallocator in turn can be specialized for array types.
This way we could use SmartPointer for both array types and scalar types.

It may make sense to replace current implementation of SmartPointer with one
from Blink or Chromium where all such problems have already been solved. WDYT?

[Sven Panne, 2013/12/04 07:40:57]

I think this class is actually trying to be a (poor & buggy) replacement for
std::unique_ptr, perhaps we can use that instead. If there is some portability
problem, we could at least try to mimic unique_ptr's interface, so we can switch
easily in the future.

I am not sure what Chrome provides and if there are any portability problems,
but I think Jochen knows (adding him)...

[alph, 2013/12/04 07:44:48]

How about making the SmartPointerBase destructor protected?
Actually nobody should have reference it via the base class.

[jochen (gone - plz use gerrit), 2013/12/04 08:07:47]

Yes, making it protected makes sense. Since this is a template thing and not a
virtual class hierarchy it should not be virtual.

 
-  inline T* operator->() const { return p_; }
+  T* operator->() const { return p_; }
 
-  // You can get the underlying pointer out with the * operator.
-  inline T* operator*() { return p_; }
+  T& operator*() const { return *p_; }
+
+  T* get() const { return p_; }
 
   // You can use [n] to index as if it was a plain pointer.
-  inline T& operator[](size_t i) {
+  T& operator[](size_t i) {
     return p_[i];
   }
 
   // You can use [n] to index as if it was a plain pointer.
-  const inline T& operator[](size_t i) const {
+  const T& operator[](size_t i) const {
     return p_[i];
   }
 
@@ -76,13 +77,14 @@ class SmartPointerBase {
   // If you want to take out the plain pointer and don't want it automatically
   // deleted then call Detach().  Afterwards, the smart pointer is empty
   // (NULL).
-  inline T* Detach() {
+  T* Detach() {
     T* temp = p_;
     p_ = NULL;
     return temp;
   }
 
-  inline void Reset(T* new_value) {
+  void Reset(T* new_value) {
+    ASSERT(p_ == NULL || p_ != new_value);

[alph, 2013/12/03 14:15:03]

maybe: if (p_ == new_value) return;

[yurys, 2013/12/03 14:28:20]

This should never happen in practice. In fact support for self reset have been
removed from scoped_ptr in Chromium as it caused some tricky errors (see
http://crbug.com/162971).

     if (p_) Deallocator::Delete(p_);
     p_ = new_value;
   }
@@ -90,7 +92,7 @@ class SmartPointerBase {
   // Assignment requires an empty (NULL) SmartArrayPointer as the receiver. Like
   // the copy constructor it removes the pointer in the original to avoid
   // double freeing.
-  inline SmartPointerBase<Deallocator, T>& operator=(
+  SmartPointerBase<Deallocator, T>& operator=(
       const SmartPointerBase<Deallocator, T>& rhs) {
     ASSERT(is_empty());
     T* tmp = rhs.p_;  // swap to handle self-assignment
@@ -99,7 +101,7 @@ class SmartPointerBase {
     return *this;
   }
 
-  inline bool is_empty() { return p_ == NULL; }
+  bool is_empty() const { return p_ == NULL; }
 
  private:
   T* p_;
@@ -119,10 +121,10 @@ struct ArrayDeallocator {
 template<typename T>
 class SmartArrayPointer: public SmartPointerBase<ArrayDeallocator<T>, T> {
  public:
-  inline SmartArrayPointer() { }
-  explicit inline SmartArrayPointer(T* ptr)
+  SmartArrayPointer() { }
+  explicit SmartArrayPointer(T* ptr)
       : SmartPointerBase<ArrayDeallocator<T>, T>(ptr) { }
-  inline SmartArrayPointer(const SmartArrayPointer<T>& rhs)
+  SmartArrayPointer(const SmartArrayPointer<T>& rhs)
       : SmartPointerBase<ArrayDeallocator<T>, T>(rhs) { }
 };
 
@@ -138,10 +140,10 @@ struct ObjectDeallocator {
 template<typename T>
 class SmartPointer: public SmartPointerBase<ObjectDeallocator<T>, T> {
  public:
-  inline SmartPointer() { }
-  explicit inline SmartPointer(T* ptr)
+  SmartPointer() { }
+  explicit SmartPointer(T* ptr)
       : SmartPointerBase<ObjectDeallocator<T>, T>(ptr) { }
-  inline SmartPointer(const SmartPointer<T>& rhs)
+  SmartPointer(const SmartPointer<T>& rhs)
       : SmartPointerBase<ObjectDeallocator<T>, T>(rhs) { }
 };