[PATCH] Initial clang-tidy architecture

Doug Gregor dgregor at apple.com
Tue Jun 4 13:12:16 PDT 2013


  The general architecture looks good here. A few comments:

  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.

  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.


================
Comment at: clang-tidy/google/GoogleModule.cpp:37
@@ +36,3 @@
+    if (!Ctor->isExplicit() && !Ctor->isImplicit() &&
+        Ctor->getNumParams() == 1) {
+      SourceLocation Loc = Ctor->getLocation();
----------------
This won't handle constructors that have > 1 arguments where the second and later arguments have defaults. Use Ctor->getNumParams() >= 1 && Ctor->getMinRequiredArguments()  <= 1

================
Comment at: clang-tidy/google/GoogleModule.cpp:42
@@ +41,3 @@
+          tooling::Replacement(*Result.SourceManager, Loc, 0, "explicit "));
+      Context->reportError(ClangTidyError(
+          *Result.SourceManager, Loc,
----------------
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?

================
Comment at: clang-tidy/llvm/LLVMModule.cpp:46
@@ +45,3 @@
+      // FIXME: Check comment content.
+      IsProperlyTerminated = true;
+    }
----------------
Early return here.

================
Comment at: clang-tidy/llvm/LLVMModule.cpp:51
@@ +50,3 @@
+      if (!ND->isAnonymousNamespace())
+        Fix = Fix.append(" ").append(ND->getNameAsString());
+      tooling::Replacements Replacements;
----------------
else "Fix = " // anonymous namespace"; ?


http://llvm-reviews.chandlerc.com/D884



More information about the cfe-commits mailing list