Committed as r160265.<div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jul 16, 2012 at 2:29 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">lgtm...<br>
<br>
we'll need to look how to make this work with the RefactoringTool next.<br>
<div class="HOEnZb"><div class="h5"><br>
On Mon, Jul 16, 2012 at 1:41 PM, Alexander Kornienko <<a href="mailto:alexfh@google.com">alexfh@google.com</a>> wrote:<br>
><br>
><br>
><br>
> On Fri, Jul 13, 2012 at 8:17 PM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
>><br>
>> On Fri, Jul 13, 2012 at 3:45 PM, Alexander Kornienko <<a href="mailto:alexfh@google.com">alexfh@google.com</a>><br>
>> 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<br>
>> > decided<br>
>> > to put off large-scale documentation efforts for now and first see how<br>
>> > it<br>
>> > goes in practical sense.<br>
>><br>
>> +++ tools/clang/test/Tooling/clang-ast-dump.cpp (revision 0)<br>
>> There's an easier way to use this with the FixedCompilationDatabase (--<br>
>> -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.<br>
><br>
> Done, reduced to a reasonable minimum.<br>
><br>
>><br>
>> +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>
><br>
> Done.<br>
><br>
>><br>
>> Also, any reason not to have the constructor take argc & argv, as the<br>
>> function doesn't return errors anyway?<br>
><br>
> Yep, the reason is to allow user to add some help text after the common help<br>
> text added in CommandLineClangTool constructor and before parsing<br>
> command-line options.<br>
><br>
>> + cl::extrahelp MoreHelp(MoreHelpText);<br>
>><br>
>> Any reasons not to put this as a static global?<br>
><br>
> Same as the previous one.<br>
><br>
>><br>
>> + delete Compilations;<br>
>><br>
>> Any reasons not to make this an OwningPtr?<br>
><br>
> No serious reasons.<br>
>><br>
>> + Compilations = NULL;<br>
>><br>
>> What would that help in the destructor?<br>
><br>
> Idiom "delete p; p = NULL;", so a pointer is always valid or null. Anyway,<br>
> now it's OwningPtr.<br>
><br>
>><br>
>> Cheers,<br>
>> /Manuel<br>
>><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<br>
>> >> 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<br>
>> >> <<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<br>
>> > 221 77<br>
>> > 957<br>
>> > Google Germany GmbH | Dienerstr. 12 | 80331 München<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>