Re: [boost] [smart_ptr] enable_shared_from_this enhancement (patch)

From: Frank Mori Hess (frank.hess_at_[hidden])
Date: 2008-03-18 16:31:33

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Tuesday 18 March 2008 15:13 pm, Peter Dimov wrote:
> I liked your original formulation with two shared_ptr instances more. It's
> better to avoid direct use of shared_count. The aliasing constructor may
> prove helpful.

I only switched to using a shared_count because that's what 
sp_enable_shared_from_this() took as an argument.  I started to change 
sp_enable_shared_from_this to take a shared_ptr argument instead of pn/px, 
but then started to get nervous that I didn't fully understand why all the 
code was the way it was and decided to just make it work with the 
shared_count.

> I don't see why pn needs to be made public.

It doesn't and shouldn't be, that was just me wanting to quickly get it to 
compile to prove to myself that the scheme would really work.

> The sp_enable_shared_from_this logic needs to go into a member function
> (_init? _adopt?) of enable_shared_from_this.

Yes, and it seemed to me like _internal_weak_this (and all the additional 
members I added) should be made private, although I may be missing something.

> I'm unclear on the purpose of pn.swap(...) in sp_enable_shared_from_this.
> It seems to me that this would overwrite the user's deleter, breaking
> get_deleter. I don't see why it's needed.

Yes, I think it does break get_deleter(), I hadn't considered that.  It's done 
because all the shared_ptrs created by shared_from_this() before the object 
is owned by the user's shared_ptr need to be made consistent with the 
shared_count in the user's shared_ptr.  The ethical course would have been to 
take the shared_count from the user's shared_ptr and copy it into any 
preexisting shared_ptrs created by shared_from_this().  However, there may be 
many of those, and there was no list of references to them available.  So I 
took the practical course, and went the opposite direction by copying a 
shared_count from the preexisting shared_ptrs into the user's shared_ptr (of 
which there is only one, and which has a handy reference available).

Maybe if the deleter used by enable_shared_from_this was a class with a 
template conversion operator, which returned the deleter obtained by getting 
the user's original deleter from the _user_deleter member?  I haven't read 
through the get_deleter code for shared_ptr yet, so I don't know what's 
really practical.

> I'm disturbed by the fact that we now provide an easy (beginner-accessible)
> way to create dangling pointers. Consistent shared_ptr use is supposed to
> make this hard.

A sanity check could be added to the destructor of enable_shared_from_this, 
something like

BOOST_ASSERT(owned() || _internal_shared_this.unique());

That would ensure the user either properly passed ownership of the object to a 
shared_ptr, or at least didn't leave any dangling pointers created by 
shared_from_this() around.

> I'd be interested to hear your ideas about the new precondition of
> shared_from_this and its new thread safety.

Honestly, I've tried to suppress thoughts of thread-safety so far.  I just 
repeat to myself "it will all work out fine" to make the bad thoughts go 
away.

- -- 
Frank
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFH4Cav5vihyNWuA4URAkGCAJ9g5ifnycNr5ffPaQfGVCM8orG7IwCdE84D
9RMoSkMm4p2uMG+YkTQNLvE=
=Mh3a
-----END PGP SIGNATURE-----