[cfe-commits] [PATCH] AST matchers

[Manuel Klimek]

On Thu, Jul 5, 2012 at 11:37 PM, Douglas Gregor <dgregor at apple.com> wrote:

>
> On Jul 5, 2012, at 1:11 PM, Manuel Klimek <klimek at google.com> wrote:
>
> 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.
>
>
> Okay, works for me. Patch looks good!
>

Thx!
Submitted as r159805.
Next up: Documentation.

Cheers,
/Manuel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120706/b5657b34/attachment.html>