Re: [boost] shared_ptr improvement proposal

From: Rob Stewart (stewart_at_[hidden])
Date: 2004-08-06 10:04:14

From: Tylo <tylo_at_[hidden]>
> 
> I propose this usage (for shared_ptr):
> 
>    shared_ptr<Widget> wp1 = NULL; // 1a
>    shared_ptr<Widget> wp2 = rawptr(new Widget(a, b)); // 1b
>    wp1 = rawptr( new Widget(a, b) ); // 1c
>    wp2 = NULL; // 1d
>    
> Current shared_ptr usage:
> 
>    shared_ptr<Widget> wp1( NULL ); // 2a
>    shared_ptr<Widget> wp2( new Widget(a, b) ); // 2b
>    wp1.reset( new Widget(a, b) ); // 2c
>    wp2.reset(); // 2d

Lines 1a and 2a are just the difference between assignment and
initialization syntax.  The former can be slower than the latter,
though the difference may be optimized away.  Nevertheless, your
code changes don't favor one over the other.

> 1. As wp1 and wp2 should be raw pointer "look alikes", the proposed usage is 
> more intuitive than the current usage, in my opinion, and allows to assign 
> NULL without side-effects. 

Assigning the null pointer constant (whether named "NULL" or "0")
in lieu of a no-argument reset() is quite reasonable.

> 2. As noted by others [e.g. Andrei Alexandrescu], member functions in smart 
> pointers should be avoided if possible. The reset() function makes it hard to 
> switch between raw and smart pointers, and also allows mix-ups: wp1.reset(), 
> (*wp1).reset(), wp1->reset().

I agree that reset() should not be a mf (1c and 1d versus 2c and
2d).  However, reset() could also be implemented as a non-member
function:

   reset(wp1, ...);

That has the advantage of being made to work identically for all
pointer types via specialization.

> 3. The rawptr() function makes initialization and assignment explicit using an 
> alternative and better syntax, IMO. E.g., you can verify that every shared_ptr 
> owns a valid raw pointer by investigating instances of "rawptr(" in the code. 
> Typically, you want to see: rawptr(new SomeClass(..)). To do this is not easy 
> with the current implementation. You have to search for:
>    a) "shared_ptr<" stack variable constructions of which takes raw pointers 
> as arguments.
>    b) ".reset(" for re-assigning a new raw pointer. Unfortunately reset() is a 
> commonly used name.
>    c) shared_ptr's in class member initiation lists, as mentioned in the 
> example presented.

Line 1b is a means to make construction from raw pointers more
apparent, which is good, I agree.  Note that your version can be
rewritten like this, too:

   shared_ptr<Widget> p(rawptr(new Widget(a, b)));

Note, however, that "rawptr" is rather terse for something you're
trying to call attention to.  How about "from_raw_ptr" or
"from_raw_pointer?"

> 4. Most importantly, all raw pointers that are assigned to smart pointers must 
> go through the rawptr() function (or rawptr_t<T> type), always making 
> it "explicit". The problem in the example presented earlier, is therefore 
> eliminated.

Right.

> 6. The only obvious problem I see, is the backward compability issue, although 
> it still support the "unprotected but powerful pointer constructor approach", 
> with a slightly different syntax.

The standardized version doesn't have to worry about backward
compatiblity, fortuntely, so these ideas could be made part of
that version, even if there is difficulty making the change to
boost::smart_ptr.  The interesting thing is that these changes
could be made an extension to the current boost::smart_ptr so
that safer usage is possible, just not required.

> Implementation for shared_ptr
> =============================
> 
> rawptr.hpp (new file):
> ---------------------
> namespace boost {
> 
>   template <class T> class rawptr_t {
>       public:
>          explicit rawptr_t(T * p) : px(p) {}
>          T* get() const { return px; }
>       private:
>          rawptr_t& operator=(const rawptr_t&);
>          T* px;
>    };

Why not just:

   template <typename T>
   struct rawptr_t
   {
      T * p_;
   };

You started with a raw pointer, and you're just trying to give it
a special type recognized by shared_ptr.  Why clutter it with the
rest of the baggage?

-- 
Rob Stewart                           stewart_at_[hidden]
Software Engineer                     http://www.sig.com
Susquehanna International Group, LLP  using std::disclaimer;