[cfe-commits] [PATCH] AST matchers

[Manuel Klimek]

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!
>

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120629/9b9c8077/attachment.html>