[cfe-commits] [PATCH] AST matchers

Douglas Gregor dgregor at apple.com
Thu Jul 5 14:37:14 PDT 2012


On Jul 5, 2012, at 1:11 PM, Manuel Klimek <klimek at google.com> wrote:

> On Fri, Jun 29, 2012 at 10:40 PM, Douglas Gregor <dgregor at apple.com> wrote:
> 
> On Jun 15, 2012, at 5:49 AM, Manuel Klimek <klimek at google.com> 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.
> 
> Switched to camelCase, as discussed later in this thread an on irc.
>  
> 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? 
> 
> Done. ASTMatchers don't depend on Frontend any more. Much nicer now.
>  
> 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?
> 
> As discussed on irc, we copy the maps (recursive algorithm that accumulates the ID -> Node mappings and triggers the callbacks). Until we have a better algorithm, we keep the maps.
>  
> In general, there are a lot of places where we could/should be using StringRef rather than std::string. Might even help performance somewhat :)
> 
> I've changed all where it was easily possible. The matcher macros still use std::string, as they store the type as members. 
> 
> 
> 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.
> 
> Chandler convinced me that we should go into that direction. While converting CamelCase to camelCase I already applied a more AST-like naming scheme to the matchers.

Okay, works for me. Patch looks good!

	- Doug

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120705/46d52f7c/attachment.html>


More information about the cfe-commits mailing list