[llvm] r290720 - [ADT] Rewrite IntrusiveRefCntPtr's comments. NFC
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 3 11:39:18 PST 2017
On Mon, Jan 2, 2017 at 12:06 PM Justin Lebar <jlebar at google.com> wrote:
> > 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.
>
Sure thing - sent D28245 <https://reviews.llvm.org/D28245>
>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170103/873a973d/attachment.html>
More information about the llvm-commits
mailing list