[PATCH] Initial clang-tidy architecture

[Chandler Carruth]

  So, I have two high-level comments. The first is pretty easy, the second one may be a more significant comment...

  1) I'd like to see the design and architecture of this get documented pretty reasonably up-front. Yes, I'm sure it will change, and I'm not trying to tie us to anything, mostly trying to get us to develop the design incrementally the same as the code. I also think that will help others understand both where we are and where we're going. I'm thinking the standard .rst based documentation that we use elsewhere in Clang so that it can grow and become the document that MySpecialCompany would read if they want to implement a module of checkers for their internal conventions.

  2) I think that it is a really big mistake to invent yet another diagnostic mechanism here. We already have **3** diagnostic mechanisms in Clang! One from LLVM that is used (in part) to diagnose inline assembly, one for Clang's diagnostics, and a third for the static analyzer.

  Now, to be fair, the third one is the most reasonable -- the static analyzer wants to present *very* detailed information, including domain-specific information such as control flow and data flow predicates which result in the bug.

  But the other two are just separate because when building out the Clang diagnostic engine, we didn't do the proper work to factor the LLVM one sufficiently that the Clang complexity could be naturally layered on top of the basic framework of LLVM's diagnostics. I would like to avoid duplicating that mistake here. ;]

  To that end, I would suggest directly using Clang's diagnostics, and where they are not sufficiently factored into a re-usable form, refactoring them to arrive at that useful form. If you're really motivated, you could also look at factoring them to be a layer on top of the LLVM diagnostics, but I think that is a larger (and largely orthogonal) project. As part of this, the diagnostic messages will move into tables with generated code via tablegen. I think this is actually A Good Thing because it will get you translation capabilities out of the box.

  I think the primary reason to not pursue using Clang's diagnostic machinery is that it is a lot of work to factor it out sufficiently, but I would advocate strongly that we pay that upfront cost.


  Besides these issues, I have some more detailed comments on the patch as it stands below.


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

================
Comment at: clang-tidy/ClangTidyModule.h:22
@@ +21,3 @@
+typedef std::pair<StringRef, ClangTidyCheck *> NamedCheck;
+typedef SmallVectorImpl<NamedCheck> ClangTidyCheckVector;
+
----------------
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.

================
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",
----------------
Stale comment?

================
Comment at: clang-tidy/google/GoogleModule.h:1
@@ +1,2 @@
+//===--- LLVMModule.h - clang-tidy ------------------------------*- C++ -*-===//
+//
----------------
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.

================
Comment at: clang-tidy/google/GoogleModule.cpp:1
@@ +1,2 @@
+//===--- LLVMModule.cpp - clang-tidy ----------------------------*- C++ -*-===//
+//
----------------
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.

================
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);
----------------
Hmm, this seems almost like too much of a prototype to me... But ok.


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