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

Thanks!
/Manuel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120705/1590157b/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ast-matchers.patch
Type: application/octet-stream
Size: 216771 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120705/1590157b/attachment.obj>