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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
