[boost] Proposal for fixing dangerous shared_ptr c'tor problem + syntax improvements

From: Tyge Løvset (Tyge.Lovset_at_[hidden])
Date: 2002-05-02 10:24:16

Hi all,
 
I would like to bring up, and suggest a solution to a problem
with most current smart pointer implementations,
(boost::shared_ptr, std::auto_ptr, ...). I would guess that
this problem has been discussed at some point in this forum,
but it is still there.
The proposed solution also opens up for more natural 
smart pointer syntax.
 
THE PROBLEM

   class X {...};
     
   class A {
      public: A(X* x) : x_(x) {}
      ...
      private: X* x_;
   };
      
   X* x = new X;
   ...
   A a1 A(x);
   ...
   delete x;
 
Now, let's say a non-expert changes x_ to a shared_ptr:
 
   class A {
      public: A(X* x) : x_(x) {}
      ...
      private: boost::shared_ptr<X> x_;
   };
   
   X* x = new X;
   ...
   A a1(x);
   
Here, he forgot to change A's constructor along with share_ptr.
Only by inspecting the complete code we spot the bug that
has been introduced. The *real* problem, however, is that
the current code still compiles and works correctly!!

At some later time:
 
   X* x = new X;
   ...
   A a1(x);
   ...
   A a2(x); // now, both a1 and a2 tries to delete x.
   

ANALYSIS 
 
The reason for the problem is that the constructor of shared_ptr
accept a single raw pointer. The 'explicit' keyword does
not help us here, because constructor initializations is always
explicit anyway. A similar example is the following illegal construct:
 
   return boost::shared_ptr<X>(this);
 
One possible prevention of the problem is to add a dummy argument to the 
share_ptr constructor (e.g. int): We then get strange user code, like:
   
   boost::shared_ptr<UserObject> sp(new UserObject(x, y, z), 0);
   
 
PROPOSED SOLUTION
 
A water tight solution, which also gives room for more natural
smart pointer syntax, is the following:

Allocator class (simulates operator new):
 
   template <typename T>
   class new1 : private non_copyable {
      public:
         new1() : p_(new T) {}         
         template <typename A> new1(const A& a) : p_(new T(a)) {}        
         template <typename A, typename B> new1(const A& a, const B& b) : p_(new T(a,b)) {}
         // ... up to e.g. 10 arguments            
            
         T* release() const { assert(p_); T* t = p_; p_ = 0; return t; }
      private:
         mutable T* p_;
   };
 

Now: 
   template <typename Y>
   shared_ptr(const new1<Y>& m)
      : px(m.release()) { pn = detail::shared_count(px, deleter()); } // hmm
      
   shared_ptr(null_struct* = 0) 
      : px(0), pn(0, deleter()) {}
      
      
We now get safe and natural user code:
 
   shared_ptr<UserObject> sp1 = new1 <UserObject>(x, y, z);
   shared_ptr<UserObject> sp2 = NULL;
   
Note that new1 allows the UserObject c'tor arguments of  to be of the following:
value types, const pointers, non-const pointers or const-references, but
not non-const references (very rare in constructors anyway,
and probably a bad practice).
 
The reset(T* = 0) method could be be replaced by the more natural and safe methods:
 
   shared_ptr& operator=(null_struct*)      // ex: sp = NULL;
      { this_type().swap(*this); return *this; }
and:   

   shared_ptr& operator=(const new1<Y>& m)  // ex: sp = new1 <UserObject>(a, b, c);
      { this_type(m.release()).swap(*this); return *this; }

SUMMARY

I ensure that a bad raw pointer is never given to a shared_ptr,
and allow to make shared_ptr syntax more natural (i.e. similar to raw pointers)
 
Any comments?

Regards,
Tyge Lovset
http:://www.cmr.no