[llvm] r290720 - [ADT] Rewrite IntrusiveRefCntPtr's comments. NFC
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 2 11:55:57 PST 2017
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170102/279b6912/attachment.html>
More information about the llvm-commits
mailing list