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! > Thanks for reviewing! > 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. > That fell out when I did the automatic refactoring of all functions to llvmFunctionStyle with the exception of matchers - I didn't do a perfect job there, but thought that I wanted to clean that up when I know which direction to go... I'll make them all CamelCase then. > 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? > Good point. We'll still want to be able to easily get a FrontendAction given a matcher... /me wonders whether we can create an interface to put somewhere else that we depend on. Ideas? > 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 :) > Yea, all where performance matters were already converted - it got us approx. 10% performance improvement. I'll convert the rest, too and report back with an updated patch. > 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. > I'm not sure yet why exactly, but to me whole words are *much* easier to read - my brain can do pattern matching on words, but abbrs in general slow me down (perhaps because I'm not a native speaker - I definitely have less problems with that in German). Now I understand that that's subjective, but I'll put some fight into being able to keep non-abbreviated names in the matchers. Or beg. Probably beg. Puh-lease? :) Cheers, /Manuel
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
