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! - Doug
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
