[llvm] r290720 - [ADT] Rewrite IntrusiveRefCntPtr's comments. NFC
Justin Lebar via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 2 11:59:34 PST 2017
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