Re: [boost] Re: SmartPtr (Loki) - auto_ptr/move c'tor issue

From: David Abrahams (dave_at_[hidden])
Date: 2003-01-30 14:03:35

"David B. Held" <dheld_at_[hidden]> writes:

> "David Abrahams" <dave_at_[hidden]> wrote in message
> news:uptqfrm97.fsf_at_boost-consulting.com...
>> > [...]
>> > Here, r_ can't tell if foo is being destructed, or just r_, and that
>> > could be a very important difference.
>>
>> It usually isn't.  I'm tempted to assert that it shouldn't be.
>> When does that distinction ever matter?
>
> Well, if r_ is being destroyed because of an exception, and it owns a
> resource, it needs to clean it up, because it is the only one that knows
> about the resource.  If r_ is being destroyed normally, and under
> normal circumstances, cleans up its resources before the d'tor is
> called (nor should clean them up in the d'tor for the normal case),
> then the d'tor *shouldn't* clean up resources (because it could be
> doing so prematurely).  I assume this happens any time a type has
> ownership transfer (move) semantics.
>
>> > Even if only one (sub)object is being initialized with a resource,
>>
>> I don't think I ever suggested that should be the case.  However,
>> one (sub)object should have responsibility for managing the
>> resource at any given time.
>
> But if that (sub)object is nested within another object, then you are
> going to run into the same exception issues we have here, even if you
> are trying to practice "True RAII".
>
>> [...]
>> DWIM == "Do What I Mean."  My sense was that you weren't yet
>> sure what you really wanted,
>
> Well, it seems obvious what I wanted: if any policy c'tor throws, a
> magical routine would descend from heaven and clean up the mess.

That's a very imprecise description, and exactly what I mean by "not
sure what you really wanted."  Even though you think you have an
answer now, I want to encourage you to write down very carefully what
you think should happen in case an exception occurs at each possible
point.  It might even tell you why your current design is "obviously"
the right one.

>> and that if you wrote it down carefully at least one of the two
>> following things would become apparent:
>
> Well, I think we all wrote it in several ways, few of which turned out
> to be very useful.

I don't think it was every written precisely.

>>    1. How to do it in the current language
>
> Believe it or not, I think I have a solution. ;)
>
>>    2. That if you changed C++ to make it adjust for this case it would
>>       undermine important guarantees the language currently provides.
>
> I don't think my exceptional d'tors would undermine important language
> guarantees, but I'm not sure they're really necessary now, either.

I guess we can debate that when you find a case where they're needed
;-)

>> > but if I have to specialize for move semantics anyway, I might be
>> > able to kill two birds with one stone.
>>
>> Don't know what that's about, but I'm not sure I need to.
>
> Well, your solution requires me to create a wrapper on top of the
> current smart_ptr class, 

Or a wrapper underneath it, depending on how you look at it.

> which is a major PITA.  

I can't judge that part, but I can say that this is an incredibly useful thing
to do, as hinted at near the bottom of
http://www.gotw.ca/gotw/008.htm (search for "base class").  It's one
reason my implementation of an exception-safe STL was a lot cleaner
(and more-correct) than the original SGI implementation was.

> But in order to solve the problem with too many c'tors for the move
> configuration, 

"Lots of constructors" is one of the main reasons to factor it into a
resource-managing base class and an interface-providing derived
facade.

> it looks like I'll have to create a partial specialization for move
> and non-move versions, which of course, calls for a common base
> class anyway.  So if I'm going to do the work of creating another
> layer of indirection, I might as well get two things from it.
>
>> [...]
>> I don't think this is a new idea.  It's a simple for of
>> transfer-of-ownership.
>
> Yup.  It's a member-initializer version of ScopeGuard(TM).

I love it when people put trademarked names on trivial things I've
been doing since at least '96.  Well, alright, maybe I don't ;-)

>> [...]
>> I think this breaks what is a very clean model, and I'm tempted to
>> assert that classes which would take advantage of this are badly
>> designed.  Why should scalar_storage be destroyed differently
>> based on whether some arbitrary sibling member or derived class
>> constructor throws an exception?
>
> Namely, because it takes ownership of a resource which will be
> transferred away, shared, or cleaned up before its d'tor is called in
> the normal case, but which must be cleaned up immediately in the
> exceptional case.

That's a very unusual thing to do, though it does arise occasionally.
The "badly designed" part of what I'm saying arises because that is
not a property of the class, but of how it is being used.  Attaching
that behavior to the class itself poisons its for use in "normal"
scenarios.

> Basically, I was trying to avoid having to create another layer of
> indirection (which is a lot of work!).  Here's my new solution:
> Integrate the deleter with the subobject that takes ownership,
> or: *arrange for cleanup to happen in the d'tor*.
>
> storage_policy(stored_type const& p) : pointee_(p)
> { }
>
> ~storage_policy::storage_policy()
> {
>     boost::checked_delete(pointee_);
> }
>
> void storage_policy::release()
> {
>     pointee_ = 0;
> }
>
> smart_ptr::smart_ptr(P p)
>     : storage_policy(p), ownership(p), checking(), conversion()
> {
> }
>
> smart_ptr::~smart_ptr()
> {
>     if (!ownership_policy::release(get_impl(*this)))
>     {
>         storage_policy::release();
>     }
> }
>
> Now, storage_policy(P) must simply clean up p if it wants to throw.
> No problem there (current impl doesn't throw anyway).  All we did
> was reverse the logic so that storage_policy always attempts to clean
> up, and we tell it *not* to if it shouldn't.  

It'll be interesting to see how well optimizers do with smart_ptr
destructors, i.e.  will they be able to deduce that pointee_ has
already been nulled out and eliminate the test and call to
checked_delete, and will they be able to then detect that the
smart_ptr is being destroyed anyway and optimize away the setting of
pointee_ to 0?  All theoretically possible.  We may need to give
checked_delete a throw() specification on some platforms to get there.

> RAII to the rescue!  

Of course ;-)

> Now storage_policy is behaving like a well-mannered auto_ptr<>
> child.  No base impl necessary, no unnatural no-throw guarantees
> required, no goofy helper classes involved, and it's as
> exception-safe as I know how to make it.

Yes, nice job (I think -- a clear specification of what "cleanup"
means at each stage is still missing).  

We'll have to see about efficiency.  My approach was biased towards
paying in the constructor where things are probably expensive anyway
and leaving the destructor as simple as possible.

> Dave, I hereby dub thee "Prophet of RAII". ;>  And Andrei is now
> "Prophet of Orthogonality".  By not straying from these guiding
> principles, the optimal solution has been found!  Hallelujah!

It's a good start, for sure.  "Optimal" is a bit premature I think.
Rejoicing is perfectly appropriate however ;->

> P.S. What you said earlier now makes perfect sense.  The problem
> was that both storage and ownership were trying to own the resource
> cooperatively.  That led to the peculiar situation that storage acquired
> the resource, and ownership controlled its destruction, which makes
> for difficult exception safety.  Now, we let storage own the resource,
> and we relinquish control to ownership at the 11th hour, if ownership
> tells us that it's really still a shared resource.  As far as storage is
> concerned, it's transferring its possession to ownership.  

This might express that notion better, though I don't remember enough
about the policies to say whether it's correct (also, you might want
to rename one of the 'release' functions, since they seem to do
different things):

    smart_ptr::~smart_ptr()
    {
        ownership_policy::release(storage_policy::release());
    }

> One resource controlled by one (sub)object.  I think I finally get
> it. ;)

It's a good mantra, isn't it?

-- 
                       David Abrahams
   dave_at_[hidden] * http://www.boost-consulting.com
Boost support, enhancements, training, and commercial distribution