shared_ptr improvement proposal by T. Lovset --- note: Parts of this may be long-winded and possible obvious to some of you, but I feel it is needed to support the arguments. Let me know what you think. --- I propose a modification to the smart pointer classes. In my opinion, this modification improves the current smart pointer usage in several respects, although it is not completely backward compatible. However, that is also the main purpose of the modification, because as we know, the explicit constructors that take a raw pointer is dangerous in numerous ways. As an illustration, I show a realistic example that violates the smart pointer preconditions in a subtle (and dangerous) manner. As a background, some quotes from the boost "Proposal to Add General Purpose Smart Pointers to the Library Technical Report" (http://std.dkuug.dk/jtc1/sc22/wg21/docs/papers/2003/n1450.html): (1)
A common concern, dating back to [Colvin94], is that it is very easy to violate the preconditions of smart pointer operations. This proposal attempts to systematically eliminate undefined behavior possibilities as long as the user is manipulating already valid shared_ptr (and weak_ptr) instances, but using the pointer constructor correctly is still up to the user.
- Widespread existing practice strongly favors the unprotected but powerful
And: pointer constructor
approach.
(2)
A. General Principles 1. "As Close to Raw Pointers as Possible, but no Closer"
For auto_ptr and shared_ptr, we are familiar with the form: shared_ptr<Widget> smartWidgetPtr( new Widget(a, b) ); The constructor is made "explicit" in order to avoid implicit and unintentional construction of smart pointers, e.g.: void f( shared_ptr<Widget> const & wp ); would without explicit constructors accept f(rawWidgetPtr) with disastrous results. Still, as mentioned, "explicit" does not make up for all misuses. For example, an inexperienced user could try to call f(shared_ptr<Widget> (rawWidgetPtr)) to make it compile. Fortunately, this is quite easy to spot as an error, and therefore "explicit" does it's mission here. But a much more common and subtle problem may arise when converting from raw pointers to smart pointers: class WidgetUtil { public: WidgetUtil(Widget* wp); private: Widget* m_wp; }; WidgetUtil::WidgetUtil(Widget* wp) : m_wp(wp) { ... } Now converting to shared_ptr: class WidgetUtil { public: WidgetUtil(Widget* wp); private: shared_ptr<Widget> m_wp; }; The unmodified constructor still compiles, but the precondition for m_wp is violated. One must therefore pay close attention to the constructors to make sure that they do not take raw pointer as arguments, which initializes smart pointers. In the case of a big class with a mixture of both smart and raw pointer members, the principle of "using the pointer constructor correctly is still up to the user", lays an unacceptable heavy burden upon the user (in my opinion) - especially when it is possible to avoid this problem. Before I move on to the proposal, I'll address another drawback with the current smart pointer implementations: implicit ctor-initialization and assignment to NULL (which is possible with raw pointers) is not supported. This would make things much easier when converting back and forth between smart and raw pointers. It also breaks the principle of "As Close to Raw Pointers as Possible, but no Closer". My proposal makes this possible, and thus making the usage closer to that of raw pointers, without introducing any side effects. [As a side note, Joe Guttman pointed out to me that with the proposed nullptr and nullptr_t, it will be easy to add these features to the current implementations of smart pointers. However, my proposal does not rely on nullptr.] The Proposal ============ Note that this proposal would be beneficial to most smart pointer implementation, including shared_ptr and auto_ptr. Unfortunately, auto_ptr is already in the standard, so it can't be touched. I propose this usage (for shared_ptr): shared_ptr<Widget> wp1 = NULL; shared_ptr<Widget> wp2 = rawptr(new Widget(a, b)); wp1 = rawptr( new Widget(a, b) ); wp2 = NULL; Current shared_ptr usage: shared_ptr<Widget> wp1( NULL ); shared_ptr<Widget> wp2( new Widget(a, b) ); wp1.reset( new Widget(a, b) ); wp2.reset(); Discussion: 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. 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(). 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. 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. 5. This proposal introduces no additional overhead, as long as the compiler really inline the rawptr() function (the operator new call is dominant by an order of magnitude in any case). 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. 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; }; template <class T> inline rawptr_t<T> rawptr(T * p) { return rawptr_t<T>(p); } } shared_ptr.hpp (diff from boost 1.31.0): ---------------------------------------- *************** *** 109,114 **** --- 123,129 ---- // Borland 5.5.1 specific workaround typedef shared_ptr<T> this_type; + struct null_tag {}; public: *************** *** 117,130 **** typedef T * pointer; typedef typename detail::shared_ptr_traits<T>::reference reference; ! shared_ptr(): px(0), pn() // never throws in 1.30+ { } template<class Y> ! explicit shared_ptr(Y * p): px(p), pn(p, checked_deleter<Y>()) // Y must be complete { ! detail::sp_enable_shared_from_this(p, p, pn); } // --- 132,145 ---- typedef T * pointer; typedef typename detail::shared_ptr_traits<T>::reference reference; ! shared_ptr(null_tag* = NULL): px(0), pn() // never throws in 1.30+ { } template<class Y> ! shared_ptr(rawptr_t<Y> const & w): px(w.get()), pn(w.get(), checked_deleter<Y>()) // Y must be complete { ! detail::sp_enable_shared_from_this(w.get(), w.get(), pn); } // *************** *** 226,231 **** --- 241,258 ---- } #endif + shared_ptr & operator=(null_tag *) + { + reset(); + return *this; + } + + template<class Y> + shared_ptr & operator=(rawptr_t<Y> const & w) + { + this_type(w).swap(*this); + return *this; + } void reset() // never throws in 1.30+ { *************** *** 235,241 **** template<class Y> void reset(Y * p) // Y must be complete { BOOST_ASSERT(p == 0 || p != px); // catch self-reset errors ! this_type(p).swap(*this); } template<class Y, class D> void reset(Y * p, D d) --- 262,268 ---- template<class Y> void reset(Y * p) // Y must be complete { BOOST_ASSERT(p == 0 || p != px); // catch self-reset errors ! this_type(rawptr(p)).swap(*this); } template<class Y, class D> void reset(Y * p, D d) ------------------------------------------------------------ Få din egen @start.no-adresse gratis på http://www.start.no/
From: Tylo <tylo@start.no>
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@sig.com Software Engineer http://www.sig.com Susquehanna International Group, LLP using std::disclaimer;
On Aug 6, 2004, at 5:04 PM, Rob Stewart wrote:
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?"
Somehow it has got stuck in my head that this should be shared_pointer_cast<>, but I'm having trouble justifying it, other than that it would look nice next to the dynamic_pointer_cast<> and so forth. shared_ptr<Widget> p = shared_ptr_cast<Widget>(new Widget); A little wordy, but if you want the event to stand out as Rob suggests, that would do it. In this case it would be handy: Base *someFactory(int typetomake); shared_ptr<Derived> d = shared_ptr_cast<Derived>(someFactory(DERIVED)); I also like the ability to assign to NULL. To me it looks pretty obviously like the right thing to do... -t
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@sig.com Software Engineer http://www.sig.com Susquehanna International Group, LLP using std::disclaimer; _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
From: "troy d.straszheim" <troy@resophonic.com>
On Aug 6, 2004, at 5:04 PM, Rob Stewart wrote:
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?"
Somehow it has got stuck in my head that this should be shared_pointer_cast<>, but I'm having trouble justifying it, other than that it would look nice next to the dynamic_pointer_cast<> and so forth.
shared_ptr<Widget> p = shared_ptr_cast<Widget>(new Widget);
Actually, that's better because there's the potential to overload/specialize it for other source pointer types, and because from_raw_pointer(), as suggested, can't be used for any other destination type and yet purports to be the only such conversion you'll need. IOW, "from_raw_pointer" doesn't mention the destination type, while "shared_ptr_cast" does. OTOH, "from_raw_pointer" could be made to be like C++ casts in order to make it interoperable with other destination types: shared_ptr<Widget> p(from_raw_pointer<shared_ptr<Widget> >( new Widget)); Once you see that, you really want to get "_cast" in the name: shared_ptr<Widget> p(raw_ptr_cast<shared_ptr<Widget> >( new Widget)); ("pointer" versus "ptr" would really make it long.) -- Rob Stewart stewart@sig.com Software Engineer http://www.sig.com Susquehanna International Group, LLP using std::disclaimer;
troy d.straszheim wrote:
[...] Somehow it has got stuck in my head that this should be shared_pointer_cast<>, but I'm having trouble justifying it, other than that it would look nice next to the dynamic_pointer_cast<> and so forth.
shared_ptr<Widget> p = shared_ptr_cast<Widget>(new Widget); [...]
I kinda like the idea as well. In a sense, you *are* converting the type of the pointer returned by new from a raw pointer to a shared_ptr<>. However, I wonder how much benefit would be derived from this change in practice. This would break all existing shared_ptr<> code, wouldn't it? Dave
On Aug 7, 2004, at 3:14 AM, David B. Held wrote:
troy d.straszheim wrote:
shared_ptr<Widget> p = shared_ptr_cast<Widget>(new Widget); [...]
I kinda like the idea as well. In a sense, you *are* converting the type of the pointer returned by new from a raw pointer to a shared_ptr<>. However, I wonder how much benefit would be derived from this change in practice. This would break all existing shared_ptr<> code, wouldn't it?
As Rob notes, there's no reason the current practice couldn't remain legal: shared_ptr<Widget> p(new Widget); // still OK but not "best practice" but of course that falls short of the mark w.r.t. *forcing* the "announcement" of the conversion from raw to shared_ptr via the shared_ptr_cast<>. I find Rob's observation that the shared_ptr_cast<> supports specialization for other source types (I knew something good and obvious was there, duh) compelling. Would seem reasonable to me to allow both and have the docs reflect that a cast is preferred, but if the consensus is to turn off the constructor-from-raw-pointer, if not now, when? As I'm new to the list, I don't have a good feel for the cost of this in terms of client-code-breakage, nor for how much emphasis is placed on backwards-compatibility in boost releases... (relatively little, since the idea is to hammer out standards-ready designs?) -t
troy d.straszheim wrote:
Would seem reasonable to me to allow both and have the docs reflect that a cast is preferred, but if the consensus is to turn off the constructor-from-raw-pointer, if not now, when?
There is no consensus to turn off the raw pointer constructor. While replacements are frequently being proposed - some of them predating boost::shared_ptr - none of them have proven useful in practice; or if they have, I am not aware of them. For now I'm inclined to think of this recurring idea as something that looks good on paper but doesn't offer significant benefits. Of course I may be wrong. ;-) Another alternative is to tackle the problem at runtime. In a "safe mode" with some help from the heap manager, it's possible for the pointer constructor to detect invalid arguments (the current implementation has a proof of concept). However, interestingly enough, none of the "safe mode" STLs offer such safeguards for std::auto_ptr (to the best of my knowledge); there's probably no demand.
On Aug 11, 2004, at 1:07 PM, Peter Dimov wrote:
There is no consensus to turn off the raw pointer constructor. While replacements are frequently being proposed - some of them predating boost::shared_ptr - none of them have proven useful in practice; or if they have, I am not aware of them. For now I'm inclined to think of this recurring idea as something that looks good on paper but doesn't offer significant benefits. Of course I may be wrong. ;-)
OK this is exactly what I was wondering about. Thanks... -t
Rob Stewart <stewart@sig.com> writes:
From: Tylo <tylo@start.no>
I propose this usage (for shared_ptr):
shared_ptr<Widget> wp1 = NULL; // 1a
<snip>
Current shared_ptr usage:
shared_ptr<Widget> wp1( NULL ); // 2a
Lines 1a and 2a are just the difference between assignment and initialization syntax.
It's the difference between copy initialization and direct initialization.
The former can be slower than the latter, though the difference may be optimized away.
On many compilers where there is a difference, copy initialization is actually faster than direct initialization. -- Dave Abrahams Boost Consulting http://www.boost-consulting.com
From: David Abrahams <dave@boost-consulting.com>
Rob Stewart <stewart@sig.com> writes:
From: Tylo <tylo@start.no>
I propose this usage (for shared_ptr):
shared_ptr<Widget> wp1 = NULL; // 1a
<snip>
Current shared_ptr usage:
shared_ptr<Widget> wp1( NULL ); // 2a
Lines 1a and 2a are just the difference between assignment and initialization syntax.
It's the difference between copy initialization and direct initialization.
Sure. I'd forgotten the terms and didn't look them up. Thanks for the clarification.
The former can be slower than the latter, though the difference may be optimized away.
On many compilers where there is a difference, copy initialization is actually faster than direct initialization.
Call me confused. copy-initialization results in the construction of an object that is then used to direct-initialize the object (wp1, in this case). Of course, the compiler is permitted to elide the extra step, so copy-initialization can degenerate into direct-initialization. How, then, can copy-initialization be faster than direct-initialization on some compilers and which are they? -- Rob Stewart stewart@sig.com Software Engineer http://www.sig.com Susquehanna International Group, LLP using std::disclaimer;
participants (6)
-
David Abrahams -
David B. Held -
Peter Dimov -
Rob Stewart -
troy d.straszheim -
Tylo