On Jun 15, 2012, at 5:49 AM, Manuel Klimek <[email protected]> wrote:
> ... one of the final pieces that we have in the tooling branch are the AST
> matchers.
>
> The main user interface is in ASTMatchers.h and ASTMatchFinder.h.
>
> Note that the AST matchers are completely independent of the tooling
> framework, but they are easy to plug into the tooling framework (see
> ASTMatchersTest.cpp).
>
> (There's also a dynamic way to contruct matchers in the works by Samuel
> Benzaquen, but it's not included in this patch, as it is a pure addition to
> the proposed matcher library)
>
> We've been using the library to drive large scale changes, metrics and
> analysis internally for > 1 year now.
This is really cool stuff. I have a few comments below, but basically I think
it's fine for this to go into the tree. Thank you!
> One of the more nit-picky review questions that I don't know what to do about
> is style:
> Matchers are an in-language DSL. We use meta-programming techniques to be
> able to contruct readable expressions without the need to spell out the types
> all the time. This leads to the matcher constructions being boths
> free-standing functions and call-able classes.
> Internally, we've used CamelCase with an upper case first letter for the
> matchers, but I'm fine with changing it either way.
CamelCase is fine for these. I notice that there's some inconsistency with
'has' names, e.g., hasDescendant vs. HasType.
I'm a little sad that MatchFinder requires me to produce a FrontendAction to
actually do the matching, because it feels like unnecessarily coupling.
MatchFinder really just needs to be handed an ASTContext and told to "walk
this", doesn't it?
I see a lot of std::map<std::string, <something>>. Do you actually need the
ordering provided by a map, or can you use llvm::StringMap instead? In general,
there are a lot of places where we could/should be using StringRef rather than
std::string. Might even help performance somewhat :)
Some of the matcher names are *really* long, and it's going to make uses of the
library look more complicated than they should. The use of "Expression" rather
than "Expr", for example, avoids name conflicts but is really painful on the
eye. Could we just drop the "Expression" suffix in most of those cases, for
example? Heck, I'm fine with renaming some of the AST classes if it would make
things easier. For example, MemberExpr -> MemberRefExpr would let us just use
MemberRef for the matcher.
- Doug
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
