ADT: IntrusiveRefCntPtr: Broaden the definition of correct usage of RefCountedBase

[dblaikie]

Summary

This roughly matches the semantics of std::enable_shared_from_this - that it
does not dictate the ownership model of all users, but constrains those users
taking advantage of the intrusive nature to do so only when there's a guarantee
that that's the ownership model being used for the object being passed.

[jlebar]

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.

[dblaikie]

Remove the contentious comment entirely rather than trying to rephrase it.

[jlebar]

It also appears in the example above; you may want to get rid of it there too.

llvm/trunk/include/llvm/ADT/IntrusiveRefCntPtr.h

 // that inherits from (ThreadSafe)RefCountedBase deletes itself when its
 // refcount hits zero.
 //
 // For example:
 //
 //   class MyClass : public RefCountedBase<MyClass> {};
 //
 //   void foo() {
-//     // Objects that inherit from RefCountedBase should always be instantiated
-//     // on the heap, never on the stack.
+//     // Constructing an IntrusiveRefCntPtr increases the pointee's refcount by
+//     // 1 (from 0 in this case).
 //     IntrusiveRefCntPtr<MyClass> Ptr1(new MyClass());
 //
 //     // Copying an IntrusiveRefCntPtr increases the pointee's refcount by 1.
 //     IntrusiveRefCntPtr<MyClass> Ptr2(Ptr1);
 //
 //     // Constructing an IntrusiveRefCntPtr has no effect on the object's
 //     // refcount.  After a move, the moved-from pointer is null.
 //     IntrusiveRefCntPtr<MyClass> Ptr3(std::move(Ptr1));

 namespace llvm {
 
 /// A CRTP mixin class that adds reference counting to a type.
 ///
 /// The lifetime of an object which inherits from RefCountedBase is managed by
 /// calls to Release() and Retain(), which increment and decrement the object's
 /// refcount, respectively.  When a Release() call decrements the refcount to 0,
 /// the object deletes itself.
-///
-/// Objects that inherit from RefCountedBase should always be allocated with
-/// operator new.
 template <class Derived> class RefCountedBase {
   mutable unsigned RefCount = 0;
 
 public:
   RefCountedBase() = default;
   RefCountedBase(const RefCountedBase &) : RefCount(0) {}
 
   void Retain() const { ++RefCount; }