[cfe-commits] r42260 - in /cfe/trunk/include/clang/Analysis/Support: ./ IntrusiveSPtr.h

[Ted Kremenek]

Hey Chris,

This class is no longer needed for anything, so I'm just going to  
remove it.  If we want to include smart reference counting pointer, we  
can just use intrusive_ptr or shared_ptr from boost (as we did with  
shared_ptr).  I originally developed this (I believe) for the  
PersistentMap class, the predecessor of ImmutableMap.  That class is  
now dead (and removed), and so there is no need for this class anymore.

Thanks for the feedback though.  It was useful.

Ted

On Dec 3, 2007, at 11:03 PM, Chris Lattner wrote:

> On Sep 23, 2007, at 11:10 PM, Ted Kremenek wrote:
>> Author: kremenek
>> Date: Mon Sep 24 01:10:20 2007
>
> Hi Ted, this is almost certainly guaranteed to be a 'page fault' :)
>
>> Added smart pointer class "IntrusiveSPtr" that handles reference
>> counted objects that maintain their own internal reference count.
>> This smart pointer implementation is compatible with LLVM-style
>> down-casting (see in llvm: include/llvm/Support/Casting.h).
>>
>> Implemented "RefCounted", a base class that objects that wish to be
>> managed using IntrusiveSPtrs can subclass.
>>
>> Reference counted objects are being targeted for use in path- 
>> sensitive
>> dataflow analyses where managing many live objects becomes difficult.
>
> Some feedback on the current version of this file:
>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> = 
>> =====================================================================
>> --- cfe/trunk/include/clang/Analysis/Support/IntrusiveSPtr.h (added)
>> +++ cfe/trunk/include/clang/Analysis/Support/IntrusiveSPtr.h Mon  
>> Sep 24 01:10:20 2007
>
> I think this should go in the main LLVM Support tree, not into  
> clang.  It is the only file in include/clang/Analysis/Support
>
>> +//===--- IntrusiveSPtr.h - Smart Reference Counting Pointers ---- 
>> *- C++ -*-===//
>
> I don't think IntrusiveSPtr is a great name for this.  really it's  
> an "intrusive ref counted pointer", how about IntrusiveRefCntPtr<>   
> or IRefCntPtr<> ?
>
>> +// This file defines two classes: RefCounted, a generic base class  
>> for objects
>> +// that wish to have their lifetimes managed using reference  
>> counting, and
>
> How about RefCountBase, RefCountedClass or just RefCount?   
> "RefCounted" is a property of a class, not a class itself.  Classes  
> should generally be nouns not adjectives.
>
>> +// IntrusiveSPtr, a template class that implements a "smart"  
>> pointer for
>> +// objects that maintain their own internal reference count (e.g.  
>> RefCounted).
>
> ok.
>
>> +// IntrusiveSPtr is similar to Boost's intrusive_ptr with two main  
>> distinctions:
>> +//
>> +//  (1) We implement operator void*() instead of operator bool()  
>> so that
>> +//      different pointer values may be accurately compared within  
>> an
>> +//      expression.  This includes the comparison of smart  
>> pointers with their
>> +//      "unsmart" cousins.
>> +//
>> +//  (2) We provide LLVM-style casting, via cast<> and dyn_cast<>,  
>> for
>> +//      IntrusiveSPtrs.
>
> Comparing to boost isn't useful for people who don't know boost.   
> Please describe the important properties here.  One important  
> property is that intrusive types automatically get virtual  
> destructors, which is suboptimal and can be solved.  See below.
>
>> +#ifndef LLVM_CLANG_INTRUSIVESPTR
>> +#define LLVM_CLANG_INTRUSIVESPTR
>> +
>> +#include "llvm/Support/Casting.h"
>
> You shouldn't need this.
>
>> +namespace clang {
>> +
>> +/// RefCounted - A generic base class for objects that wish to have
>> +///  their lifetimes managed using reference counts.  Classes  
>> subclass
>> +///  RefCounted to obtain such functionality, and are typically
>> +///  handled with IntrusiveSPtr "smart pointers" (see below) which
>> +///  automatically handle the management of reference counts.   
>> Objects
>> +///  that subclass RefCounted 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.
>> +class RefCounted {
>> +  unsigned ref_cnt;
>> +public:
>> +  RefCounted() : ref_cnt(0) {}
>> +
>> +  void Retain() { ++ref_cnt; }
>> +  void Release() {
>> +    assert (ref_cnt > 0 && "Reference count is already zero.");
>> +    if (--ref_cnt == 0) delete this;
>> +  }
>> +
>> +protected:
>> +  // Making the dstor protected (or private) prevents RefCounted
>> +  // objects from being stack-allocated.  Subclasses should  
>> similarly
>> +  // follow suit with this practice.
>> +  virtual ~RefCounted() {}
>> +};
>
> This is suboptimal because it endows the derived class with a  
> virtual dtor.  This virtual dtor is required in order for the  
> "delete this" to free the derived class data.  Thus you use it with  
> "class foo : public RefCounted {...}".  Instead, how about changing  
> this to be:
>
> template<typename Derived>
> class RefCountedClass {
> ...
>
>   if (--ref_cnt == 0) delete static_cast<Derived*>(this);
> ...
> }
>
> and then using "class foo : public RefCounted<foo> {...}"
>
> This allows both polymorphic or non-polymorphic refcounted classes  
> ("foo" can choose to have a virtual dtor or not).
>
>> +/// intrusive_ptr_add_ref - A utility function used by IntrusiveSPtr
>> +///  to increment the reference count of a RefCounted object.  This
>> +///  particular naming was chosen to be compatible with
>> +///  boost::intrusive_ptr, which provides similar functionality to
>> +///  IntrusiveSPtr.
>> +void intrusive_ptr_add_ref(clang::RefCounted* p) { p->Retain(); }
>> +
>> +/// intrusive_ptr_release - The complement of intrusive_ptr_add_ref;
>> +///  decrements the reference count of a RefCounted object.
>> +void intrusive_ptr_release(clang::RefCounted* p) { p->Release(); }
>
> boost::intrusive_ptr compatibility isn't particularly useful for  
> llvm, is it?
>
>> +namespace clang {
>> +
>> +/// IntrusiveSPtr - 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
>> +///  intrusive_ptr_add_ref/intrusive_ptr_release.  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.
>> +template <typename T>
>> +class IntrusiveSPtr {
>> +  T* Obj;
>> +public:
>> +  typedef T ObjType;
>> +
>> +  explicit IntrusiveSPtr() : Obj(NULL) {}
>> +
>> +  explicit IntrusiveSPtr(const T* obj) : Obj(const_cast<T*>(obj)) {
>
> Why cast away the const here?  Instead of doing this, just mark the  
> ref_cnt field above 'mutable'.
>
>> +// 
>> = 
>> = 
>> = 
>> ----------------------------------------------------------------------= 
>> ==//
>> +// LLVM-style downcasting support for IntrusiveSPtr objects
>> +// 
>> = 
>> = 
>> = 
>> ----------------------------------------------------------------------= 
>> ==//
>> +
>> +namespace llvm {
>> +
>> +/// cast<X> - Return the argument parameter (wrapped in an
>> +/// IntrusiveSPtr smart pointer) to the specified type.  This  
>> casting
>> +/// operator asserts that the type is correct, so it does not  
>> return a
>> +/// NULL smart pointer on failure.  Note that the cast returns a
>> +/// reference to an IntrusiveSPtr; thus no reference counts are
>> +/// modified by the cast itself.  Assigning the result of the cast,
>> +/// however, to a non-reference will obviously result in reference
>> +/// counts being adjusted when the copy constructor or operator=
>> +/// method for IntrusiveSPtr is invoked.
>>
>> +template <typename X, typename Y>
>> +inline clang::IntrusiveSPtr<X>&
>> +cast(const clang::IntrusiveSPtr<Y>& V) {
>> +  assert (isa<X>(V.getPtr()) &&
>> +    "cast<Ty>() (IntrusiveSPtr) argument of incompatible type!");
>> +  clang::IntrusiveSPtr<Y>& W =  
>> const_cast<clang::IntrusiveSPtr<Y>&>(V);
>> +  return reinterpret_cast<clang::IntrusiveSPtr<X>&>(W);
>> +}
>
> You don't need to do any of this.  :)  Instead, just specify a  
> partial specialization of llvm::simplify_type like qualtype does.   
> See ~ Type.h:173 for the implementation.  If you specify that, all  
> the builtin cast/dyn_cast etc stuff will just magically work.
>
> -Chris