On Fri, Jul 13, 2012 at 8:17 PM, Manuel Klimek <[email protected]> wrote:
> On Fri, Jul 13, 2012 at 3:45 PM, Alexander Kornienko <[email protected]> > 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 <[email protected]> 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 < > [email protected]> > >> 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 > >> > [email protected] > >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >> > > > > > > > > > > > -- > > Alexander Kornienko | Software Engineer | [email protected] | +49 151 > 221 77 > > 957 > > Google Germany GmbH | Dienerstr. 12 | 80331 München > > > -- Alexander Kornienko | Software Engineer | [email protected] | +49 151 221 77 957 Google Germany GmbH | Dienerstr. 12 | 80331 München
clang-tools.diff
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
