<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jul 13, 2012 at 8:17 PM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Fri, Jul 13, 2012 at 3:45 PM, Alexander Kornienko <<a href="mailto:alexfh@google.com">alexfh@google.com</a>> wrote:<br>

> I've moved out common behavior and data for all command-line tools to a<br>
> separate class (CommandLineClangTool.cpp), added more help, added one<br>
> integration test for clang-ast-dump, added links to the recently added<br>
> tooling setup how to. This should address most of the issues. I've decided<br>
> to put off large-scale documentation efforts for now and first see how it<br>
> goes in practical sense.<br>
<br>
</div>+++ tools/clang/test/Tooling/clang-ast-dump.cpp (revision 0)<br>
There's an easier way to use this with the FixedCompilationDatabase (-- -c...)<br>
<br>
I think that test is pretty brittle. I'd try to test the things we<br>
care about: CXXMethod.*theMethod and BinaryOperator.*+, and stuff like<br>
that. We want to minimize the chance that layout changes break the<br>
test.</blockquote><div>Done, reduced to a reasonable minimum.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+class CommandLineClangTool {<br>
+  CompilationDatabase *Compilations;<br>
+  llvm::cl::opt<std::string> BuildPath;<br>
+  llvm::cl::list<std::string> SourcePaths;<br>
+  llvm::cl::extrahelp MoreHelp;<br>
<br>
I usually prefer to have the public part first, as that's what's<br>
relevant to the user.<br>
Also, please add at least a short class comment and a comment on<br>
initialize and run (for example, it's important to mention that<br>
initialize does command line argument parsing).<br></blockquote><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, any reason not to have the constructor take argc & argv, as the<br>
function doesn't return errors anyway?<br></blockquote><div>Yep, the reason is to allow user to add some help text after the common help text added in CommandLineClangTool constructor and before parsing command-line options.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+  cl::extrahelp MoreHelp(MoreHelpText);<br>
<br>
Any reasons not to put this as a static global?<br></blockquote><div>Same as the previous one.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+  delete Compilations;<br>
<br>
Any reasons not to make this an OwningPtr?<br></blockquote><div>No serious reasons. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+  Compilations = NULL;<br>
<br>
What would that help in the destructor?<br></blockquote><div> Idiom "delete p; p = NULL;", so a pointer is always valid or null. Anyway, now it's OwningPtr.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
Cheers,<br>
/Manuel<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
<br>
><br>
><br>
> On Thu, Jul 12, 2012 at 9:57 PM, Sean Silva <<a href="mailto:silvas@purdue.edu">silvas@purdue.edu</a>> wrote:<br>
>><br>
>> +//===- examples/Tooling/ClangCheck.cpp - Clang check tool<br>
>> -----------------===//<br>
>><br>
>> The file header is still the "ClangCheck" one.<br>
>><br>
>> +//  This file implements a clang-ast-dump tool that dumps specified parts<br>
>> +//  of an AST of a number of translation units.<br>
>><br>
>> This header comment is rather uninformative (as are many others in the<br>
>> clang codebase, unfortunately).<br>
>><br>
>> It gives me a bit of a taste of what the tool does, but leaves me<br>
>> hanging. The parts that I think "leave me hanging" are:<br>
>><br>
>> "dumps *specified* parts" (emphasis mine): how do I specify them? what<br>
>> things are matchable? can I use a regex? can I use the new ASTMatcher<br>
>> library (although IIRC the dynamic matchers are not merged yet, so the<br>
>> answer here is no)?<br>
>><br>
>> "of a number of translation units": how do I specify them? do I need<br>
>> some kind of special setup? if so, how so I set it up?<br>
>><br>
>> As a person interested in using this tool, and maybe hacking on it, I<br>
>> would like to at least see:<br>
>><br>
>> * A high level description of the tool. As in all writing, remember<br>
>> your audience: they have already seen the filename, so they can<br>
>> already guess that it is related to "dumping ASTs"; thus a "high level<br>
>> description" is going to be one level lower than "it dumps ASTs".<br>
>> * Expected use cases and why this tool is needed (e.g. comparison with<br>
>> `clang -ast-dump`)<br>
>> * Quickstart. If a nontrivial setup is required, provide pointers to<br>
>> the relevant documentation.<br>
>> * Future directions you envision. One can only provide useful insight<br>
>> into future directions after grokking the tool; hence the original<br>
>> author is usually the best person to enunciate them. This usually<br>
>> provides insight into the context which brought the tool into<br>
>> existence and gives future hackers leads on where to continue.<br>
>><br>
>> Thanks for the cool tool,<br>
>> --Sean Silva<br>
>><br>
>> On Thu, Jul 12, 2012 at 10:27 AM, Alexander Kornienko <<a href="mailto:alexfh@google.com">alexfh@google.com</a>><br>
>> wrote:<br>
>> > Hi,<br>
>> ><br>
>> > This patch adds the clang-ast-dump tool based on the Clang Tooling<br>
>> > infrastructure. It can help users of AST matchers to explore and<br>
>> > understand<br>
>> > AST by selectively dumping it. This is a first version aimed at<br>
>> > collecting<br>
>> > feedback and feature requests.<br>
>> ><br>
>> > --<br>
>> > Regards,<br>
>> > Alexander<br>
>> ><br>
>> > _______________________________________________<br>
>> > cfe-commits mailing list<br>
>> > <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>> ><br>
><br>
><br>
><br>
><br>
> --<br>
> Alexander Kornienko | Software Engineer | <a href="mailto:alexfh@google.com">alexfh@google.com</a> | +49 151 221 77<br>
> 957<br>
> Google Germany GmbH | Dienerstr. 12 | 80331 München<br>
><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div><div><font color="#666666"><span style="border-top-width:2px;border-right-width:0px;border-bottom-width:0px;border-left-width:0px;border-top-style:solid;border-right-style:solid;border-bottom-style:solid;border-left-style:solid;border-top-color:rgb(213,15,37);border-right-color:rgb(213,15,37);border-bottom-color:rgb(213,15,37);border-left-color:rgb(213,15,37);padding-top:2px;margin-top:2px">Alexander Kornienko |</span><span style="border-top-width:2px;border-right-width:0px;border-bottom-width:0px;border-left-width:0px;border-top-style:solid;border-right-style:solid;border-bottom-style:solid;border-left-style:solid;border-top-color:rgb(51,105,232);border-right-color:rgb(51,105,232);border-bottom-color:rgb(51,105,232);border-left-color:rgb(51,105,232);padding-top:2px;margin-top:2px"> Software Engineer |</span></font><span style="border-top-width:2px;border-right-width:0px;border-bottom-width:0px;border-left-width:0px;border-top-style:solid;border-right-style:solid;border-bottom-style:solid;border-left-style:solid;border-top-color:rgb(0,153,57);border-right-color:rgb(0,153,57);border-bottom-color:rgb(0,153,57);border-left-color:rgb(0,153,57);padding-top:2px;margin-top:2px"><font color="#666666"> </font><a href="mailto:alexfh@google.com" style="color:rgb(17,85,204)" target="_blank">alexfh@google.com</a> |</span><span style="border-top-width:2px;border-right-width:0px;border-bottom-width:0px;border-left-width:0px;border-top-style:solid;border-right-style:solid;border-bottom-style:solid;border-left-style:solid;border-top-color:rgb(238,178,17);border-right-color:rgb(238,178,17);border-bottom-color:rgb(238,178,17);border-left-color:rgb(238,178,17);padding-top:2px;margin-top:2px"> <a value="+35315435283" style="color:rgb(17,85,204)">+49 151 221 77 957</a></span></div>
</div><div><font color="#666666"><span style="background-color:rgb(255,255,255);font-family:Arial,Verdana,sans-serif">Google Germany GmbH | </span><span style="background-color:rgb(255,255,255);font-family:Arial,Verdana,sans-serif">Dienerstr. 12 | </span><span style="background-color:rgb(255,255,255);font-family:Arial,Verdana,sans-serif">80331 München</span></font></div>
<br>
</div>