<div dir="ltr">On Tue, Jun 4, 2013 at 10:12 PM, Doug Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br><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"><br>
  The general architecture looks good here. A few comments:<br>
<br>
  1) I agree with Chandler that we want to avoiding adding another diagnostic mechanism to Clang. There's a lot of extra baggage that each diagnostic mechanism brings, and I'd much rather we extend/refactor Clang's primary mechanism to support what clang-tidy needs.<br>

<br>
  2) I find the grouping of checks under specific projects (Google and LLVM) to be odd. I think what we want is just a bunch of checks with descriptive names (explicit constructors, sorted includes, end-of-namespace comments, etc.), and some very lightweight mechanism that composes a set of checks (based on their names) into a "coding convention". That will be the point of configurability later, to compose sets of checks into your own coding style. Yes, there may be some weird checks that make sense for a particular project, and we can prefix those if we have to, but overall I expect most checks to be universal.<br>
</blockquote><div><br></div><div style>One thing I expect is that with the power of clang we'll see more projects build checks that are domain specific to their internal infrastructure classes. </div><div style><br></div>
<div style>Another point is configurability. While many standards have very similar checks - for example that end of namespace comments follow a certain pattern - they all look differently. We could of course just try to find more descriptive names for what the option is, but that seems harder.</div>
<div style><br></div><div style>Or are you opposed to the general idea that we have a set of all possible checks and basically a way to filter that down by regexp, and would prefer a more declarative syntax for how to say which checks you want to run (and perhaps with what configured values)?</div>
<div style><br></div><div style>Cheers,</div><div style>/Manuel</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: clang-tidy/google/GoogleModule.cpp:37<br>
@@ +36,3 @@<br>
+    if (!Ctor->isExplicit() && !Ctor->isImplicit() &&<br>
+        Ctor->getNumParams() == 1) {<br>
+      SourceLocation Loc = Ctor->getLocation();<br>
----------------<br>
This won't handle constructors that have > 1 arguments where the second and later arguments have defaults. Use Ctor->getNumParams() >= 1 && Ctor->getMinRequiredArguments()  <= 1<br>
<br>
================<br>
Comment at: clang-tidy/google/GoogleModule.cpp:42<br>
@@ +41,3 @@<br>
+          tooling::Replacement(*Result.SourceManager, Loc, 0, "explicit "));<br>
+      Context->reportError(ClangTidyError(<br>
+          *Result.SourceManager, Loc,<br>
----------------<br>
Actually fixing this issue can break code. Is that something for the tooling infrastructure to deal with? Should it be indicated somehow in the diagnostic?<br>
<br>
================<br>
Comment at: clang-tidy/llvm/LLVMModule.cpp:46<br>
@@ +45,3 @@<br>
+      // FIXME: Check comment content.<br>
+      IsProperlyTerminated = true;<br>
+    }<br>
----------------<br>
Early return here.<br>
<br>
================<br>
Comment at: clang-tidy/llvm/LLVMModule.cpp:51<br>
@@ +50,3 @@<br>
+      if (!ND->isAnonymousNamespace())<br>
+        Fix = Fix.append(" ").append(ND->getNameAsString());<br>
+      tooling::Replacements Replacements;<br>
----------------<br>
else "Fix = " // anonymous namespace"; ?<br>
<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D884" target="_blank">http://llvm-reviews.chandlerc.com/D884</a><br>
</blockquote></div><br></div></div>