+cfe-commits, klimek, chandlerc<div><br></div><div><br><br><div class="gmail_quote">On Wed, Aug 22, 2012 at 5:17 PM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Tue, Aug 21, 2012 at 11:47 AM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br>

</div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">Even better now: no macros, no include magic and no dependency on linkage order.<div>
<div>...
</div></div></div></div><div><div><br></div><div class="im"><div><font face="courier new, monospace">+//  An example of usage:</font></div>
<div><font face="courier new, monospace">+//  @code</font></div></div></div></blockquote><div><br></div><div>Use \code instead of @code.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div><div><font face="courier new, monospace">+//  #include "llvm/Support/CommandLine.h"</font></div><div><span style="font-family:'courier new',monospace">+...</span></div><div><font face="courier new, monospace">+//  @endcode</font></div>

</div></blockquote><div><br></div><div>\endcode</div></div></div></blockquote><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>
<div><font face="courier new, monospace">...</font></div><div><font face="courier new, monospace">+extern const char *CommonHelpMessage;</font></div></div></blockquote><div><br></div><div>Could you make this either a function returning the help massage or a "const char * const CommonHelpMessage"?</div>
</div></div></blockquote><div>Thanks for noting that. Changed to const char *const.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra">
<div class="gmail_quote"><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><font face="courier new, monospace">-/// \brief A common driver for command-line Clang tools.</font></div><div><font face="courier new, monospace">+/// \brief A parser for options common to all command-line Clang tools.</font></div>


<div><font face="courier new, monospace"> ///</font></div><div><font face="courier new, monospace"> /// Parses a common subset of command-line arguments, locates and loads a</font></div><div><font face="courier new, monospace"> /// compilation commands database, runs a tool with user-specified action. It</font></div>


<div><font face="courier new, monospace"> /// also contains a help message for the common command-line options.</font></div><div><font face="courier new, monospace">-/// An example of usage:</font></div><div><font face="courier new, monospace">-/// @code</font> </div>

</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<div><font face="courier new, monospace">-/// int main(int argc, const char **argv) {</font></div><div><font face="courier new, monospace">-///   CommandLineClangTool Tool;</font></div><div><div><font face="courier new, monospace">-///   cl::extrahelp MoreHelp("\nMore help text...");</font></div>


</div><div><font face="courier new, monospace">-///   Tool.initialize(argc, argv);</font></div><div><font face="courier new, monospace">-///   return Tool.run(newFrontendActionFactory<clang::SyntaxOnlyAction>());</font></div>


<div><font face="courier new, monospace">-/// }</font></div><div><font face="courier new, monospace">-/// @endcode</font></div><div><font face="courier new, monospace">-///</font></div></div></blockquote><div><br></div></div>
<div>
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..</div></div></div></blockquote><div>Yep, moved back to class comment.</div><div>  </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div><div><font face="courier new, monospace">-class CommandLineClangTool {</font></div>
<div><font face="courier new, monospace">+class CommonOptionsParser {</font></div><div><font face="courier new, monospace"> public:</font></div><div><font face="courier new, monospace">-  /// Sets up command-line options and help messages.</font></div>


<div><font face="courier new, monospace">-  /// Add your own help messages after constructing this tool.</font></div><div><font face="courier new, monospace">-  CommandLineClangTool();</font></div><div><font face="courier new, monospace">+  // Intentionally public.</font></div>

</div></blockquote><div><br></div></div><div>"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</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<div><font face="courier new, monospace">+  llvm::OwningPtr<CompilationDatabase> Compilations;</font></div><div><font face="courier new, monospace">+  std::vector<std::string> SourcePathList;</font></div><div>


<font face="courier new, monospace"> </font></div><div><font face="courier new, monospace">   /// Parses command-line, initializes a compilation database.</font></div><div><font face="courier new, monospace">+  /// This method can change argc and argv contents.</font></div>

</div></blockquote><div><br></div></div><div>This "constructor"!? Also, consider adding \brief to this comment and additionally saying HOW argc/argv are change, e.g. consuming known flags, ..</div></div></div></blockquote>
<div>Done.</div><div>  </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
<div><font face="courier new, monospace">Index: tools/clang/tools/clang-check/ClangCheck.cpp</font></div>
<div><font face="courier new, monospace">===================================================================</font></div><div><font face="courier new, monospace">--- tools/clang/tools/clang-check/ClangCheck.cpp        (revision 162110)</font></div>


<div><font face="courier new, monospace">+++ tools/clang/tools/clang-check/ClangCheck.cpp        (working copy)</font></div></div><div><font face="courier new, monospace">...</font></div>
<div><font face="courier new, monospace"> </font></div><div><font face="courier new, monospace">-#include "llvm/Support/CommandLine.h"</font></div><div><font face="courier new, monospace">...</font></div><div><font face="courier new, monospace">+static cl::extrahelp CommonHelp(CommonHelpMessage);</font></div>

</blockquote><div><br></div><div>extrahelp is defined in CommandLine.h, right? Why do you remove the import?</div></div></div></blockquote><div>Obviously I thought it's enough to have #include in the example ;)</div>
<div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:13px;background-color:rgb(255,255,255)"><font face="courier new, monospace">+//  An example of usage:</font></div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:13px;background-color:rgb(255,255,255)">
<font face="courier new, monospace">+//  @code</font></div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:13px;background-color:rgb(255,255,255)"><font face="courier new, monospace">+//  #include "llvm/Support/CommandLine.h"</font></div>
</div><div><font face="arial, helvetica, sans-serif">Done.</font></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
<div><font face="courier new, monospace">Index: tools/clang/lib/Tooling/CommonOptionsParser.cpp</font></div><div><font face="courier new, monospace">===================================================================</font></div>


<div><font face="courier new, monospace">--- tools/clang/lib/Tooling/CommonOptionsParser.cpp     (revision 162110)</font></div><div><font face="courier new, monospace">+++ tools/clang/lib/Tooling/CommonOptionsParser.cpp     (working copy)</font></div>


<div><font face="courier new, monospace">@@ -1,4 +1,4 @@</font></div></div><div><font face="courier new, monospace">...</font></div><div class="im"><div><font face="courier new, monospace">+//  This file implements the CommonOptionsParser class used to parse common</font></div>


<div><font face="courier new, monospace">+//  command-line options for clang tools, so that they can be run as separate</font></div><div><font face="courier new, monospace">+//  command-line applications with a consistent common interface for handling</font></div>


<div><font face="courier new, monospace">+//  compilation database and input files.</font></div></div></blockquote><div><br></div><div>nit: compilation databases</div></div></div></blockquote><div>We have only one compilation database for each run, hence singular. But maybe native speakers will correct me?</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>Cheers,</div><div>Daniel</div></div></div></blockquote>
</div><br clear="all"><div><br></div>-- <br>
</div><div>Best regards,</div><div>Alexander Kornienko</div>