On Fri, Jun 29, 2012 at 10:40 PM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><br><div><div class="im"><div>On Jun 15, 2012, at 5:49 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:</div><br><blockquote type="cite">
<div style="font-family:arial,helvetica,sans-serif"><font><div>... one of the final pieces that we have in the tooling branch are the AST matchers.</div><div><br></div><div>The main user interface is in ASTMatchers.h and ASTMatchFinder.h.</div>

<div><br></div><div>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).</div>

<div><br></div><div>(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)</div><div><br>


</div><div>We've been using the library to drive large scale changes, metrics and analysis internally for > 1 year now.</div></font></div></blockquote><div><br></div></div><div>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!</div>
</div></div></blockquote><div><br></div><div>Thanks for reviewing!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>
<div class="im"><blockquote type="cite"><div style="font-family:arial,helvetica,sans-serif"><font><div>One of the more nit-picky review questions that I don't know what to do about is style:</div>

<div>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.</div>


<div>Internally, we've used CamelCase with an upper case first letter for the matchers, but I'm fine with changing it either way.</div></font></div></blockquote><br></div></div><div>CamelCase is fine for these. I notice that there's some inconsistency with 'has' names, e.g., hasDescendant vs. HasType.</div>
</div></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div>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? </div>
</div></div></blockquote><div><br></div><div>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?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>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 :)</div>
</div></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>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.</div>
</div></blockquote><div><br></div><div>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? :)</div>
<div><br></div><div>Cheers,</div><div>/Manuel</div><div><br></div></div>