[PATCH] D28245: ADT: IntrusiveRefCntPtr: Broaden the definition of correct usage of RefCountedBase

[David Blaikie via llvm-commits]

On Tue, Jan 3, 2017 at 11:43 AM Justin Lebar via Phabricator <
reviews at reviews.llvm.org> wrote:

> jlebar requested changes to this revision.
> jlebar added a comment.
> This revision now requires changes to proceed.
>
> I think what I wrote on the mailing list applies here:
>
> > btw I don't think it's at all so simple.  For example, an object
> >  might, in one of its member functions, cause itself to be
> >  retained/released.  This is perfectly valid, but of course will break
> >  you in exciting ways if you allocate the object on the stack.
> >
> > The advice of "it's safe to allocate these objects on the stack if
> >  you're careful and the object doesn't ever cause itself to be
> >  retained/released" really means "if the object doesn't *currently*
> >  ever cause itself to be retained/released".  IOW allocating such an
> >  object on the stack constrains future changes to the object's
> >  implementation.  If I had to do a cleanup of code I didn't care about
> >  that was allocating such an object on the stack before I could land a
> >  change I cared about, I'd be pretty annoyed, and rightfully so, I
> >  think.
> >
> > That is to say, unless it's part of an object's contract that it never
> >  causes itself to be retained/released, I think you're in for a bad
> >  time.  Indeed, the same applies for every function you pass that
> >  object to: Unless it is part of that function's contract that it never
> >  causes the object to be retained or released, passing a
> >  stack-allocated object to such a function is going to constrain future
> >  changes to the implementation of that function and all transitive
> >  callees.
> >
> > This seems like an extremely unfriendly thing to do.
>
> That is, as written this comment seems to say that it's OK for me to take a
> class that inherits from RefCountedBase and allocate it on the stack if I
> am
> careful about it.  But doing so constrains the author of the class from
> ever
> calling Retain/Release on itself.  I think our default should be that this
> is
> allowed, which implies that our default should be that allocating such
> objects
> on the stack is disallowed.
>

I feel like the default should be the opposite (which is
std::enable_shared_from_this's position as well) - that the need for
intrusive reference counting would /allow/ the type to be owned in this
way, but not require it. If a type requires it, it could document that (on
any specific operations, or universally).


> If a class explicitly promises never to Retain/Release itself, then
> allocating
> it on the stack is fine.  However, for the same reason, you should only
> pass it
> to functions that promise never to Retain/Release the object.
>
> I'm fine if you want to write this up in a comment, although I am curious
> what
> problem you're trying to solve.
>

Broadly: That ownership semantics are kept orthogonal to types as much as
possible. That some users of a type may need to leverage intrusive
reference counting (to get through some interface that can't accurately
model the ownership in the interim, for example) shouldn't so-constrain all
users to use the type that way.

- Dave


>
>
> https://reviews.llvm.org/D28245
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170103/26694a28/attachment.html>