[cfe-commits] [PATCH] Reverted clang-check to fully supported CommandLine Library use-case: global static variables.

[Alexander Kornienko]

+cfe-commits, klimek, chandlerc



On Wed, Aug 22, 2012 at 5:17 PM, Daniel Jasper <djasper at google.com> wrote:

>
> On Tue, Aug 21, 2012 at 11:47 AM, Alexander Kornienko <alexfh at google.com>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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120822/b818f647/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tooling-opts.diff
Type: application/octet-stream
Size: 20802 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120822/b818f647/attachment.obj>