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

[Ted Kremenek]

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*).