[llvm] r290720 - [ADT] Rewrite IntrusiveRefCntPtr's comments. NFC
Justin Lebar via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 2 12:05:59 PST 2017
> Not sure that it's so hard to enforce - usually you pick an ownership model for an object and stick with it.
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.
Nonetheless if you think this is an important distinction to carve
out, let's discuss in the context of a proposed concrete change to the
comment.
On Mon, Jan 2, 2017 at 11:59 AM, Justin Lebar <jlebar at google.com> wrote:
> It sounds like the comment is unclear. Feel free to send me a patch.
>
> On Mon, Jan 2, 2017 at 11:55 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>> On Mon, Jan 2, 2017 at 11:51 AM Justin Lebar <jlebar at google.com> wrote:
>>>
>>> Yes, it is safe to allocate an instance of an object which inherits
>>> from RefCountedBase on the stack (or whatever) so long as you never
>>> call retain().
>>>
>>> However, this is a difficult invariant to enforce, because of its
>>> global nature.
>>
>>
>> Not sure that it's so hard to enforce - usually you pick an ownership model
>> for an object and stick with it. Certainly any places where such a value is
>> treated as a value type (ie: not allocated with new directly) it'd be fine -
>> in the same way that if I create a std::string on the stack it's unlikely
>> I'll accidentally takes its address and put that address in a
>> std::unique_ptr, or the like.
>>
>>>
>>> Therefore I think it's probably good advice to say
>>> that such objects "should always be instantiated on the heap".
>>>
>>> Note that this is prescriptive, not descriptive -- it does not say
>>> that it's always unsafe to allocate such objects on the stack. I
>>> agree that would be false.
>>
>>
>> I think that may be an interesting distinction in formal english, but
>> probably isn't how the average reader might read the comment. "should
>> always" sounds fairly prescriptive (always). I think I'd certainly read
>> "should be" and "should always" as "that's the only way to use this
>> correctly". "should usually" is the wording I would expect if it was "this
>> is the right thing most of the time but there are other ways to use it".
>>
>> - Dave
>>
>>>
>>>
>>> On Mon, Jan 2, 2017 at 11:45 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>> >
>>> >
>>> > On Thu, Dec 29, 2016 at 12:10 PM Justin Lebar via llvm-commits
>>> > <llvm-commits at lists.llvm.org> wrote:
>>> >>
>>> >> Author: jlebar
>>> >> Date: Thu Dec 29 13:59:38 2016
>>> >> New Revision: 290720
>>> >>
>>> >> URL: http://llvm.org/viewvc/llvm-project?rev=290720&view=rev
>>> >> Log:
>>> >> [ADT] Rewrite IntrusiveRefCntPtr's comments. NFC
>>> >>
>>> >> Edit for voice, and also add examples. In particular, add an
>>> >> explanation for why you might want to specialize
>>> >> IntrusiveRefCntPtrInfo,
>>> >> which is not obvious.
>>> >>
>>> >> Modified:
>>> >> llvm/trunk/include/llvm/ADT/IntrusiveRefCntPtr.h
>>> >>
>>> >> Modified: llvm/trunk/include/llvm/ADT/IntrusiveRefCntPtr.h
>>> >> URL:
>>> >>
>>> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/IntrusiveRefCntPtr.h?rev=290720&r1=290719&r2=290720&view=diff
>>> >>
>>> >>
>>> >> ==============================================================================
>>> >> --- llvm/trunk/include/llvm/ADT/IntrusiveRefCntPtr.h (original)
>>> >> +++ llvm/trunk/include/llvm/ADT/IntrusiveRefCntPtr.h Thu Dec 29
>>> >> 13:59:38
>>> >> 2016
>>> >> @@ -7,14 +7,49 @@
>>> >> //
>>> >>
>>> >>
>>> >> //===----------------------------------------------------------------------===//
>>> >> //
>>> >> -// This file defines IntrusiveRefCntPtr, a template class that
>>> >> -// implements a "smart" pointer for objects that maintain their own
>>> >> -// internal reference count, and RefCountedBase, a generic base class
>>> >> -// for objects that wish to have their lifetimes managed using
>>> >> reference
>>> >> -// counting.
>>> >> +// This file defines the RefCountedBase, ThreadSafeRefCountedBase, and
>>> >> +// IntrusiveRefCntPtr classes.
>>> >> //
>>> >> -// IntrusiveRefCntPtr is similar to Boost's intrusive_ptr with added
>>> >> -// LLVM-style casting.
>>> >> +// IntrusiveRefCntPtr is a smart pointer to an object which maintains
>>> >> a
>>> >> +// reference count. (ThreadSafe)RefCountedBase is a mixin class that
>>> >> adds a
>>> >> +// refcount member variable and methods for updating the refcount. An
>>> >> object
>>> >> +// 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.
>>> >> +// 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));
>>> >> +// assert(Ptr1 == nullptr);
>>> >> +//
>>> >> +// // Clearing an IntrusiveRefCntPtr decreases the pointee's
>>> >> refcount
>>> >> by 1.
>>> >> +// Ptr2.reset();
>>> >> +//
>>> >> +// // The object deletes itself when we return from the function,
>>> >> because
>>> >> +// // Ptr3's destructor decrements its refcount to 0.
>>> >> +// }
>>> >> +//
>>> >> +// You can use IntrusiveRefCntPtr with isa<T>(), dyn_cast<T>(), etc.:
>>> >> +//
>>> >> +// IntrusiveRefCntPtr<MyClass> Ptr(new MyClass());
>>> >> +// OtherClass *Other = dyn_cast<OtherClass>(Ptr); // Ptr.get() not
>>> >> required
>>> >> +//
>>> >> +// IntrusiveRefCntPtr works with any class that
>>> >> +//
>>> >> +// - inherits from (ThreadSafe)RefCountedBase,
>>> >> +// - has Retain() and Release() methods, or
>>> >> +// - specializes IntrusiveRefCntPtrInfo.
>>> >> //
>>> >>
>>> >>
>>> >> //===----------------------------------------------------------------------===//
>>> >>
>>> >> @@ -27,16 +62,15 @@
>>> >>
>>> >> namespace llvm {
>>> >>
>>> >>
>>> >>
>>> >> -//===----------------------------------------------------------------------===//
>>> >> -/// RefCountedBase - A generic base class for objects that wish to
>>> >> -/// have their lifetimes managed using reference counts. Classes
>>> >> -/// subclass RefCountedBase to obtain such functionality, and are
>>> >> -/// typically handled with IntrusiveRefCntPtr "smart pointers" (see
>>> >> below)
>>> >> -/// which automatically handle the management of reference counts.
>>> >> -/// Objects that subclass RefCountedBase should not be allocated on
>>> >> -/// the stack, as invoking "delete" (which is called when the
>>> >> -/// reference count hits 0) on such objects is an error.
>>> >>
>>> >>
>>> >> -//===----------------------------------------------------------------------===//
>>> >> +/// 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.
>>> >
>>> >
>>> > Is this accurate? It looks like one could use a RefCountedBase without
>>> > any
>>> > release/retain calls - with any external lifetime management (stack,
>>> > unique_ptr, container, etc).
>>> >
>>> >>
>>> >> template <class Derived> class RefCountedBase {
>>> >> mutable unsigned RefCount = 0;
>>> >>
>>> >> @@ -52,18 +86,7 @@ public:
>>> >> }
>>> >> };
>>> >>
>>> >> -template <typename T> struct IntrusiveRefCntPtrInfo {
>>> >> - static void retain(T *obj) { obj->Retain(); }
>>> >> - static void release(T *obj) { obj->Release(); }
>>> >> -};
>>> >> -
>>> >> -/// \brief A thread-safe version of \c llvm::RefCountedBase.
>>> >> -///
>>> >> -/// A generic base class for objects that wish to have their lifetimes
>>> >> managed
>>> >> -/// using reference counts. Classes subclass \c
>>> >> ThreadSafeRefCountedBase
>>> >> to
>>> >> -/// obtain such functionality, and are typically handled with
>>> >> -/// \c IntrusiveRefCntPtr "smart pointers" which automatically handle
>>> >> the
>>> >> -/// management of reference counts.
>>> >> +/// A thread-safe version of \c RefCountedBase.
>>> >> template <class Derived> class ThreadSafeRefCountedBase {
>>> >> mutable std::atomic<int> RefCount;
>>> >>
>>> >> @@ -81,22 +104,37 @@ public:
>>> >> }
>>> >> };
>>> >>
>>> >>
>>> >>
>>> >> -//===----------------------------------------------------------------------===//
>>> >> -/// IntrusiveRefCntPtr - A template class that implements a "smart
>>> >> pointer"
>>> >> -/// that assumes the wrapped object has a reference count associated
>>> >> -/// with it that can be managed via calls to
>>> >> -/// IntrusivePtrAddRef/IntrusivePtrRelease. The smart pointers
>>> >> -/// manage reference counts via the RAII idiom: upon creation of
>>> >> -/// smart pointer the reference count of the wrapped object is
>>> >> -/// incremented and upon destruction of the smart pointer the
>>> >> -/// reference count is decremented. This class also safely handles
>>> >> -/// wrapping NULL pointers.
>>> >> -///
>>> >> -/// Reference counting is implemented via calls to
>>> >> -/// Obj->Retain()/Obj->Release(). Release() is required to destroy
>>> >> the
>>> >> -/// object when the reference count reaches zero. Inheriting from
>>> >> -/// RefCountedBase takes care of this automatically.
>>> >>
>>> >>
>>> >> -//===----------------------------------------------------------------------===//
>>> >> +/// Class you can specialize to provide custom retain/release
>>> >> functionality for
>>> >> +/// a type.
>>> >> +///
>>> >> +/// Usually specializing this class is not necessary, as
>>> >> IntrusiveRefCntPtr
>>> >> +/// works with any type which defines Retain() and Release() functions
>>> >> --
>>> >> you
>>> >> +/// can define those functions yourself if RefCountedBase doesn't work
>>> >> for you.
>>> >> +///
>>> >> +/// One case when you might want to specialize this type is if you
>>> >> have
>>> >> +/// - Foo.h defines type Foo and includes Bar.h, and
>>> >> +/// - Bar.h uses IntrusiveRefCntPtr<Foo> in inline functions.
>>> >> +///
>>> >> +/// Because Foo.h includes Bar.h, Bar.h can't include Foo.h in order
>>> >> to
>>> >> pull in
>>> >> +/// the declaration of Foo. Without the declaration of Foo, normally
>>> >> Bar.h
>>> >> +/// wouldn't be able to use IntrusiveRefCntPtr<Foo>, which wants to
>>> >> call
>>> >> +/// T::Retain and T::Release.
>>> >> +///
>>> >> +/// To resolve this, Bar.h could include a third header, FooFwd.h,
>>> >> which
>>> >> +/// forward-declares Foo and specializes IntrusiveRefCntPtrInfo<Foo>.
>>> >> Then
>>> >> +/// Bar.h could use IntrusiveRefCntPtr<Foo>, although it still
>>> >> couldn't
>>> >> call any
>>> >> +/// functions on Foo itself, because Foo would be an incomplete type.
>>> >> +template <typename T> struct IntrusiveRefCntPtrInfo {
>>> >> + static void retain(T *obj) { obj->Retain(); }
>>> >> + static void release(T *obj) { obj->Release(); }
>>> >> +};
>>> >> +
>>> >> +/// A smart pointer to a reference-counted object that inherits from
>>> >> +/// RefCountedBase or ThreadSafeRefCountedBase.
>>> >> +///
>>> >> +/// This class increments its pointee's reference count when it is
>>> >> created, and
>>> >> +/// decrements its refcount when it's destroyed (or is changed to
>>> >> point
>>> >> to a
>>> >> +/// different object).
>>> >> template <typename T> class IntrusiveRefCntPtr {
>>> >> T *Obj = nullptr;
>>> >>
>>> >> @@ -208,10 +246,8 @@ bool operator!=(const IntrusiveRefCntPtr
>>> >> return !(A == B);
>>> >> }
>>> >>
>>> >>
>>> >>
>>> >> -//===----------------------------------------------------------------------===//
>>> >> -// LLVM-style downcasting support for IntrusiveRefCntPtr objects
>>> >>
>>> >>
>>> >> -//===----------------------------------------------------------------------===//
>>> >> -
>>> >> +// Make IntrusiveRefCntPtr work with dyn_cast, isa, and the other
>>> >> idioms
>>> >> from
>>> >> +// Casting.h.
>>> >> template <typename From> struct simplify_type;
>>> >>
>>> >> template <class T> struct simplify_type<IntrusiveRefCntPtr<T>> {
>>> >>
>>> >>
>>> >> _______________________________________________
>>> >> llvm-commits mailing list
>>> >> llvm-commits at lists.llvm.org
>>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list