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

src/smart-pointers.h

 // Copyright 2011 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 18 matching lines @@
 #define V8_SMART_POINTERS_H_
 
 namespace v8 {
 namespace internal {
 
 
 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;
   }
 
   // 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];
   }
 
   // We don't have implicit conversion to a T* since that hinders migration:
   // You would not be able to change a method from returning a T* to
   // returning an SmartArrayPointer<T> and then get errors wherever it is used.
 
 
   // 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;
   }
 
   // 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
     const_cast<SmartPointerBase<Deallocator, T>&>(rhs).p_ = NULL;
     p_ = tmp;
     return *this;
   }
 
-  inline bool is_empty() { return p_ == NULL; }
+  bool is_empty() const { return p_ == NULL; }
 
  private:
   T* p_;
 };
 
 // A 'scoped array pointer' that calls DeleteArray on its pointer when the
 // destructor is called.
 
 template<typename T>
 struct ArrayDeallocator {
   static void Delete(T* array) {
     DeleteArray(array);
   }
 };
 
 
 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) { }
 };
 
 
 template<typename T>
 struct ObjectDeallocator {
   static void Delete(T* object) {
     delete object;
   }
 };
 
 
 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) { }
 };
 
 } }  // namespace v8::internal
 
 #endif  // V8_SMART_POINTERS_H_