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

[Justin Lebar via Phabricator via llvm-commits]

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.

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.


https://reviews.llvm.org/D28245