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

[Chris Lattner]

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