On Thu, Jul 5, 2012 at 11:37 PM, Douglas Gregor <[email protected]> wrote:
> > On Jul 5, 2012, at 1:11 PM, Manuel Klimek <[email protected]> wrote: > > On Fri, Jun 29, 2012 at 10:40 PM, Douglas Gregor <[email protected]>wrote: > >> >> 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. >> > > 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! > Thx! Submitted as r159805. Next up: Documentation. Cheers, /Manuel
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
