[PATCH] Initial clang-tidy architecture

[Daniel Jasper]

================
Comment at: clang-tidy/ClangTidyModule.h:21
@@ +20,3 @@
+
+typedef std::pair<StringRef, ClangTidyCheck *> NamedCheck;
+typedef SmallVectorImpl<NamedCheck> ClangTidyCheckVector;
----------------
Chandler Carruth wrote:
> If this type is worth giving a name, I strongly suspect it's members are worth giving names. Can we use a struct?
Done.

================
Comment at: clang-tidy/ClangTidyModule.h:22
@@ +21,3 @@
+typedef std::pair<StringRef, ClangTidyCheck *> NamedCheck;
+typedef SmallVectorImpl<NamedCheck> ClangTidyCheckVector;
+
----------------
Chandler Carruth wrote:
> I don't think this typedef adds a lot of value. I'd rather just see the SmallVectorImpl<NamedCheck> in the APIs. The nice thing about SmallVectorImpl is that it is largely abstracted away already.
Removed.

================
Comment at: clang-tidy/google/GoogleModule.h:1
@@ +1,2 @@
+//===--- LLVMModule.h - clang-tidy ------------------------------*- C++ -*-===//
+//
----------------
Chandler Carruth wrote:
> Stale comment.
> 
> Also, I find the file naming here overly generic:
> 
> GoogleModule.h and LLVMModule.h are crazy close. For example, we used to have an 'llvm/Module.h' and other things.
> 
> I would make them FooTidyModule, or FooCheckModule or something more specific to clang-tidy checks.
Done.

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

================
Comment at: clang-tidy/google/GoogleModule.cpp:42
@@ +41,3 @@
+          tooling::Replacement(*Result.SourceManager, Loc, 0, "explicit "));
+      Context->reportError(ClangTidyError(
+          *Result.SourceManager, Loc,
----------------
Doug Gregor wrote:
> 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?
I am not sure that this can be sufficiently dealt with by the tooling infrastructure. The code that breaks might be in an arbitrary different file, even in a different codebase (that happens to use this class).

So, my idea for this is to have "confidence levels" both for the error report itself and for the fix. These error levels might reach from "Guaranteed correct" to "Please take a close look". Then clang-tidy will get options -report_level and -fix_level to say: Report everything at least this certain and fix everything at least this certain.


================
Comment at: clang-tidy/google/GoogleModule.cpp:1
@@ +1,2 @@
+//===--- LLVMModule.cpp - clang-tidy ----------------------------*- C++ -*-===//
+//
----------------
Chandler Carruth wrote:
> More stale comments. I'm going to stop trying to find all of them. Please do a pass over the comments to get them accurate.
> 
> Also note that for files ending in '.cpp' you don't need the 'C++' in the top comment line. You only need that for files that end in '.h' to help Emacs out.
Fixed (I hope).

================
Comment at: clang-tidy/google/GoogleModule.cpp:70-71
@@ +69,4 @@
+
+// Register the JSONCompilationDatabasePlugin with the
+// CompilationDatabasePluginRegistry using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<GoogleModule> X("google-module",
----------------
Chandler Carruth wrote:
> Stale comment?
Fixed.

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

================
Comment at: clang-tidy/llvm/LLVMModule.cpp:51
@@ +50,3 @@
+      if (!ND->isAnonymousNamespace())
+        Fix = Fix.append(" ").append(ND->getNameAsString());
+      tooling::Replacements Replacements;
----------------
Doug Gregor wrote:
> else "Fix = " // anonymous namespace"; ?
Ah, is that the convention? I have mostly seen just "// namespace" (to which Fix is already initialized).

================
Comment at: clang-tidy/llvm/LLVMModule.cpp:87-88
@@ +86,4 @@
+                                  const Module *Imported) {
+    // FIXME: This is a dummy implementation to show how to get at preprocessor
+    // information. Implement a real include order check.
+    StringRef SourceFile = Sources.getFilename(HashLoc);
----------------
Chandler Carruth wrote:
> Hmm, this seems almost like too much of a prototype to me... But ok.
Yeah, I know :-(.. Will be one of the first things to implement after this. I wanted to get something in that shows the use of PPCallbacks, though, as I think they will be important for quite a few checks.


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