[cfe-dev] Ownership attribute for malloc etc. checking

[Andrew McGregor]

Ok, so I implemented what I think is all the suggestions...

The patch is around 100 lines shorter, so that's a definite improvement.

I'm not sure this is as minimal as it could be.

While I was there, I also changed the use of std::sort in the nonnull
parsing, since that's where I picked it up from in the first place.

Andrew

On Tue, Jun 29, 2010 at 2:43 PM, Ted Kremenek <kremenek at apple.com> wrote:

> On Jun 28, 2010, at 7:00 PM, Andrew McGregor wrote:
>
> > So, thinking about this led me to this idea:
> >
> > These attributes actually have minimal differences so far as the checker
> is concerned.  So how about there being only one attribute object  type,
> probably an OwnershipAttr, with an enum kind embedded in it, that can be
> applied to an argument.  Each one would apply to just one argument or the
> return value, but may have several arguments (the so far unimplemented
> calloc case will need this, as will pointer-to-pointer malloc).  The parser
> would have to construct two attribute objects for
> __attribute((ownership_holds, 1, 2)).  That way the dispatch can be a case
> statement on the enum rather than the current mess of casts and tests.  This
> would also save some of the methods from being templates, which I think
> would be unnecessary complexity.
> >
> > Or, again in order to reduce the duplication, I could make templates of
> some of the methods and use the type to represent the attribute kind the
> same way as I have so far.  Disadvantage: this is probably going to bloat.
>
> I'm fine with having one attribute with an enum, but I don't think it is
> mutually exclusive from having subclasses.  You can keep all the key logic
> in one class, and for clients that want to do queries that are specific to a
> particular attribute they can do downcast.  For example:
>
> class OwnershipAttr {
>  enum OwnershipKind { Holds, Takes, ... };
>  OwnershipKind OKind;
> ...
> };
>
> class OwnershipReturnsAttr : public OwnershipAttr {
> public:
>  OwnershipReturnsAttr() : OwnershipAttr(Returns) {}
>
>  static bool classof(OwnershipAttr *A) { return A->OKind == Returns; }
> };
>
> This is basically the same idiom we use with Stmts and Exprs in Clang's
> ASTS.  This gives you the best of both worlds.  You can use the 'enum' when
> you want switch on the ownership kind, but still can use the classes to
> encapsulate the relevant logic and add APIs that are only meaningful for a
> particular attribute.  For example, consider the main analysis function in
> the static analyzer.  It is littered with case statements like:
>
>   case Stmt::BlockExprClass:
>      VisitBlockExpr(cast<BlockExpr>(S), Pred, Dst);
>      break;
>
> The strength of this approach is you get strong-typing when you want it,
> but it doesn't add a burden if you don't need it (e.g., if you only care
> that something is a OwnershipAttr* instead of a OwnershipReturnsAttr*).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20100630/c86bf0e5/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-ownership-withtests-r2.patch
Type: text/x-patch
Size: 27323 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20100630/c86bf0e5/attachment.bin>