lgtm...
we'll need to look how to make this work with the RefactoringTool next.
On Mon, Jul 16, 2012 at 1:41 PM, Alexander Kornienko <ale...@google.com> wrote:
>
>
>
> On Fri, Jul 13, 2012 at 8:17 PM, Manuel Klimek <kli...@google.com> wrote:
>>
>> On Fri, Jul 13, 2012 at 3:45 PM, Alexander Kornienko <ale...@google.com>
>> wrote:
>> > I've moved out common behavior and data for all command-line tools to a
>> > separate class (CommandLineClangTool.cpp), added more help, added one
>> > integration test for clang-ast-dump, added links to the recently added
>> > tooling setup how to. This should address most of the issues. I've
>> > decided
>> > to put off large-scale documentation efforts for now and first see how
>> > it
>> > goes in practical sense.
>>
>> +++ tools/clang/test/Tooling/clang-ast-dump.cpp (revision 0)
>> There's an easier way to use this with the FixedCompilationDatabase (--
>> -c...)
>>
>> I think that test is pretty brittle. I'd try to test the things we
>> care about: CXXMethod.*theMethod and BinaryOperator.*+, and stuff like
>> that. We want to minimize the chance that layout changes break the
>> test.
>
> Done, reduced to a reasonable minimum.
>
>>
>> +class CommandLineClangTool {
>> + CompilationDatabase *Compilations;
>> + llvm::cl::opt<std::string> BuildPath;
>> + llvm::cl::list<std::string> SourcePaths;
>> + llvm::cl::extrahelp MoreHelp;
>>
>> I usually prefer to have the public part first, as that's what's
>> relevant to the user.
>> Also, please add at least a short class comment and a comment on
>> initialize and run (for example, it's important to mention that
>> initialize does command line argument parsing).
>
> Done.
>
>>
>> Also, any reason not to have the constructor take argc & argv, as the
>> function doesn't return errors anyway?
>
> 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.
>
>> + cl::extrahelp MoreHelp(MoreHelpText);
>>
>> Any reasons not to put this as a static global?
>
> Same as the previous one.
>
>>
>> + delete Compilations;
>>
>> Any reasons not to make this an OwningPtr?
>
> No serious reasons.
>>
>> + Compilations = NULL;
>>
>> What would that help in the destructor?
>
> Idiom "delete p; p = NULL;", so a pointer is always valid or null. Anyway,
> now it's OwningPtr.
>
>>
>> Cheers,
>> /Manuel
>>
>>
>>
>> >
>> >
>> > On Thu, Jul 12, 2012 at 9:57 PM, Sean Silva <sil...@purdue.edu> wrote:
>> >>
>> >> +//===- examples/Tooling/ClangCheck.cpp - Clang check tool
>> >> -----------------===//
>> >>
>> >> The file header is still the "ClangCheck" one.
>> >>
>> >> +// This file implements a clang-ast-dump tool that dumps specified
>> >> parts
>> >> +// of an AST of a number of translation units.
>> >>
>> >> This header comment is rather uninformative (as are many others in the
>> >> clang codebase, unfortunately).
>> >>
>> >> It gives me a bit of a taste of what the tool does, but leaves me
>> >> hanging. The parts that I think "leave me hanging" are:
>> >>
>> >> "dumps *specified* parts" (emphasis mine): how do I specify them? what
>> >> things are matchable? can I use a regex? can I use the new ASTMatcher
>> >> library (although IIRC the dynamic matchers are not merged yet, so the
>> >> answer here is no)?
>> >>
>> >> "of a number of translation units": how do I specify them? do I need
>> >> some kind of special setup? if so, how so I set it up?
>> >>
>> >> As a person interested in using this tool, and maybe hacking on it, I
>> >> would like to at least see:
>> >>
>> >> * A high level description of the tool. As in all writing, remember
>> >> your audience: they have already seen the filename, so they can
>> >> already guess that it is related to "dumping ASTs"; thus a "high level
>> >> description" is going to be one level lower than "it dumps ASTs".
>> >> * Expected use cases and why this tool is needed (e.g. comparison with
>> >> `clang -ast-dump`)
>> >> * Quickstart. If a nontrivial setup is required, provide pointers to
>> >> the relevant documentation.
>> >> * Future directions you envision. One can only provide useful insight
>> >> into future directions after grokking the tool; hence the original
>> >> author is usually the best person to enunciate them. This usually
>> >> provides insight into the context which brought the tool into
>> >> existence and gives future hackers leads on where to continue.
>> >>
>> >> Thanks for the cool tool,
>> >> --Sean Silva
>> >>
>> >> On Thu, Jul 12, 2012 at 10:27 AM, Alexander Kornienko
>> >> <ale...@google.com>
>> >> wrote:
>> >> > Hi,
>> >> >
>> >> > This patch adds the clang-ast-dump tool based on the Clang Tooling
>> >> > infrastructure. It can help users of AST matchers to explore and
>> >> > understand
>> >> > AST by selectively dumping it. This is a first version aimed at
>> >> > collecting
>> >> > feedback and feature requests.
>> >> >
>> >> > --
>> >> > Regards,
>> >> > Alexander
>> >> >
>> >> > _______________________________________________
>> >> > cfe-commits mailing list
>> >> > cfe-commits@cs.uiuc.edu
>> >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >> >
>> >
>> >
>> >
>> >
>> > --
>> > Alexander Kornienko | Software Engineer | ale...@google.com | +49 151
>> > 221 77
>> > 957
>> > Google Germany GmbH | Dienerstr. 12 | 80331 München
>> >
>
>
>
>
> --
> Alexander Kornienko | Software Engineer | ale...@google.com | +49 151 221 77
> 957
> Google Germany GmbH | Dienerstr. 12 | 80331 München
>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
