Re: [v3 patch] Add std::tr1::shared_ptr

[Jonathan Wakely <cow at compsoc dot man dot ac dot uk>]

On Mon, Feb 21, 2005 at 11:12:51AM -0600, Benjamin Kosnik wrote:

> 
> This looks great Jonathan.
> 
> > There are no tests for weak_ptr and incomplete tests for shared_ptr,
> > I'll add them eventually but don't have enough time to do it right now.
> 
> Sounds acceptable to me. FYI, you shouldn't need to key off of __EXCEPTIONS in the individual testsuite files.

It'd be possible to get rid of that. I think easiest would be to run it
and expect it to fail at runtime (since it would throw with exceptions
enabled or abort() without).  Does that sound OK?

> All you need to do is make sure -fno-exceptions compiles cleanly. (It
> looks like you've tested this. Am I correct?)

I did test it, but a while ago. I'll check again.

> > I have modified the relevant Boost tests to use the std::tr1::shared_ptr
> > instead of boost::shared_ptr and run them successfully, but only on a
> > single processor machine.  The threaded tests should be run on as many
> > exotic multiprocessor machines as possible, as one of the major changes
> > I made to the classes was to use GCC's atomic ops instead of the
> > Boost.Threads library.  I can't commit those tests but I'll make them
> > available to those who want them.
> 
> Can you post them after you check this in?

Certainly. You'll need to have Boost available, although removing that
dependency shouldn't be hard.

> > For the shared_ptr_timing_test my modified versions did better without
> > optimisation, but worse when optimisation was enabled.  I plan to
> > investigate that at some point.  It might be because the Boost.Threads
> > locks can be inlined whereas GCC's atomic ops will make a function call.
> 
> That would be interesting to figure out.

So would a way of implementing _Sp_counted_base::add_ref_lock() without
a race condition (oops!):

  void
  add_ref_lock()
  {
    if (_M_use_count <= 0) // XXX is this MT safe?
    {
      __throw_bad_weak_ptr();
    }
    // !!! race condition !!! 
    __gnu_cxx::__atomic_add(&_M_use_count, 1);
  }

If another thread decrements _M_use_count before the increment then the
object might have been destroyed, so we _should_ throw, but instead we
return "successfully".  I think I need a CAS such that _M_use_count is
incremented iff it's non-zero:

  void
  add_ref_lock()
  {
    if (conditional_increment(_M_use_count) == 0)
      __throw_bad_weak_ptr();
  }

> > I expect there will be plenty of changes to come but this should
> > probably be committed now and improved later. OK for mainline?
> 
> Sounds like a great plan. Please check this in to mainline.

Great!
I'll comment that function to say it's broken, and fix it a.s.a.p

jon

-- 
"I always keep a supply of liquor handy in case I see a snake,
 which I also keep handy."
	- W.C. Fields