+cfe-commits, klimek, chandlerc
On Wed, Aug 22, 2012 at 5:17 PM, Daniel Jasper <[email protected]> wrote: > > On Tue, Aug 21, 2012 at 11:47 AM, Alexander Kornienko > <[email protected]>wrote: > >> Even better now: no macros, no include magic and no dependency on linkage >> order. >> ... >> >> +// An example of usage: >> +// @code >> > > Use \code instead of @code. > > >> +// #include "llvm/Support/CommandLine.h" >> +... >> +// @endcode >> > > \endcode > We definitely need a sane doxygen style-guide for llvm/clang. Otherwise developers constantly face non-obvious choices between alternative syntaxes, and reviewers loose their time commenting on "wrong" choices. > > >> ... >> +extern const char *CommonHelpMessage; >> > > Could you make this either a function returning the help massage or a > "const char * const CommonHelpMessage"? > Thanks for noting that. Changed to const char *const. > -/// \brief A common driver for command-line Clang tools. >> +/// \brief A parser for options common to all command-line Clang tools. >> /// >> /// Parses a common subset of command-line arguments, locates and loads a >> /// compilation commands database, runs a tool with user-specified >> action. It >> /// also contains a help message for the common command-line options. >> -/// An example of usage: >> -/// @code >> > -/// int main(int argc, const char **argv) { >> -/// CommandLineClangTool Tool; >> -/// cl::extrahelp MoreHelp("\nMore help text..."); >> -/// Tool.initialize(argc, argv); >> -/// return >> Tool.run(newFrontendActionFactory<clang::SyntaxOnlyAction>()); >> -/// } >> -/// @endcode >> -/// >> > > In general, I think it makes more sense to have the comment/example usage > as a class comment here. That way, it is visible in Doxygen.. > Yep, moved back to class comment. > -class CommandLineClangTool { >> +class CommonOptionsParser { >> public: >> - /// Sets up command-line options and help messages. >> - /// Add your own help messages after constructing this tool. >> - CommandLineClangTool(); >> + // Intentionally public. >> > > "Intentionally public" is nice. Additionally giving the reason would be > even better. And, as they are now part of the public interface, they > deserve a doxygen comment > > >> + llvm::OwningPtr<CompilationDatabase> Compilations; >> + std::vector<std::string> SourcePathList; >> >> /// Parses command-line, initializes a compilation database. >> + /// This method can change argc and argv contents. >> > > This "constructor"!? Also, consider adding \brief to this comment and > additionally saying HOW argc/argv are change, e.g. consuming known flags, .. > Done. > Index: tools/clang/tools/clang-check/ClangCheck.cpp >> =================================================================== >> --- tools/clang/tools/clang-check/ClangCheck.cpp (revision 162110) >> +++ tools/clang/tools/clang-check/ClangCheck.cpp (working copy) >> ... >> >> -#include "llvm/Support/CommandLine.h" >> ... >> +static cl::extrahelp CommonHelp(CommonHelpMessage); >> > > extrahelp is defined in CommandLine.h, right? Why do you remove the import? > Obviously I thought it's enough to have #include in the example ;) +// An example of usage: +// @code +// #include "llvm/Support/CommandLine.h" Done. > > >> Index: tools/clang/lib/Tooling/CommonOptionsParser.cpp >> =================================================================== >> --- tools/clang/lib/Tooling/CommonOptionsParser.cpp (revision 162110) >> +++ tools/clang/lib/Tooling/CommonOptionsParser.cpp (working copy) >> @@ -1,4 +1,4 @@ >> ... >> +// This file implements the CommonOptionsParser class used to parse >> common >> +// command-line options for clang tools, so that they can be run as >> separate >> +// command-line applications with a consistent common interface for >> handling >> +// compilation database and input files. >> > > nit: compilation databases > We have only one compilation database for each run, hence singular. But maybe native speakers will correct me? > > Cheers, > Daniel > -- Best regards, Alexander Kornienko
tooling-opts.diff
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
