Re: [boost] shared_xxx issues

From: Greg Colvin (gcolvin_at_[hidden])
Date: 2001-09-02 09:48:05

From: Peter Dimov <pdimov_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Sunday, September 02, 2001 4:41 AM
Subject: Re: [boost] shared_xxx issues

> From: "Greg Colvin" <gcolvin_at_[hidden]>
> > From: David Abrahams <david.abrahams_at_[hidden]>
> 
> [...]
> 
> > > > > Second Issue:
> > > > >
> > > > > I found the following code in shared_array (and I assume the analogous code
> > > > > is in shared_ptr):
> > > > >
> > > > >         void reset(T* p=0)
> > > > >         {
> > > > >             if ( px == p ) return;  // fix: self-assignment safe
> > > > >
> > > > > What this does is to make a silent success of resetting a shared_ptr with
> > > > > the pointer it owns. The problem with that is that resetting a shared_ptr to
> > > > > any /other/ pointer which is managed by a shared_ptr is a fatal (and
> > > > > hard-to-detect) error, which happens even to experiencedprogrammers. I
> > > > > don't think the trade-off is a good one. Making this special case (which we
> > > > > can detect) silently work when all other cases will fail without complaint
> > > > > is a mistake. I'd rather see an assert() here.
> 
> [...]
> 
> > You've almost convinced me.  What is bugging me is a desire to
> > stay consistent with auto_ptr:
> >
> >    void reset(X* p=0) throw();
> >       Effects:
> >          If get() != p then delete get().
> >       Postconditions:
> >          *this holds the pointer p.
> 
> I think that Dave is right. I don't know why this made it into
> auto_ptr::reset, but the above shared_ptr::reset does not conform to its
> definition:

Right.  Either code the is wrong, or the definition, or both.

I don't recall who made the "fix" for self-reset, but the effect
is to make self-reset consistent with self-assignment, as it is
for auto_ptr.  I'm back to not being convinced that this is a
problem, and am inclined to fix the definition.

Just because it is possible to stumble into undefined behavior
in many ways doesn't mean we shouldn't prevent this one way.

> "shared_ptr reset
> 
> void reset( T* p=0 );
> First, if use_count() == 1, deletes the object pointed to by the stored
> pointer. Otherwise, use_count() for any remaining copies is decremented by
> 1.
> 
> Then replaces the contents of this, as if by storing a copy of p, which must
> have been allocated via a C++ new expression or be 0. Afterwards,
> use_count() is 1 (even if p==0; see ~shared_ptr). Note that in C++  delete
> on a pointer with a value of 0 is harmless.
> 
> The only exception which may be thrown is std::bad_alloc.  If an exception
> is thrown,  delete p is called."
> 
> The 'fix' makes the reset() a no-op, and this is not allowed by the
> specification. Note the effect 'decrements use_count for remaining copies'
> and the postcondition 'use_count() == 1'. (BTW an assert is not allowed
> either by this definition. The undefined behavior would happen at
> destruction, not at reset.)
> 
> That's why in my implementation (after staring at the current
> shared_ptr::reset() code for a while) I simply used
> 
> void reset(T * p = 0)
> {
>     shared_ptr(p).swap(*this);
> }