[PATCH] Initial clang-tidy architecture

[Manuel Klimek]

  Putting in mainly smaller things, as I think the overall design is fine...


================
Comment at: clang-tidy/ClangTidy.h:30
@@ +29,3 @@
+      : Message(Message), Fix(Fix) {
+    FilePath = Sources.getBufferName(Loc);
+    FileOffset = Sources.getFileOffset(Loc);
----------------
Not sure whether we might want to use getFilename().

================
Comment at: clang-tidy/ClangTidy.h:27
@@ +26,3 @@
+struct ClangTidyError {
+  ClangTidyError(const SourceManager &Sources, SourceLocation Loc,
+                 StringRef Message, const tooling::Replacements &Fix)
----------------
Not sure, but we might want a range.

================
Comment at: clang-tidy/ClangTidy.h:40
@@ +39,3 @@
+
+typedef SmallVector<ClangTidyError, 16> ClangTidyErrors;
+
----------------
Use SmallVectorImpl?

================
Comment at: clang-tidy/ClangTidy.h:42
@@ +41,3 @@
+
+/// \brief Every \c ClangTidyCheck reports errors through a conetxt.
+class ClangTidyContext {
----------------
s/conetxt/context/

================
Comment at: clang-tidy/ClangTidy.h:45
@@ +44,3 @@
+public:
+  void reportError(const ClangTidyError &Error) { Errors.push_back(Error); }
+
----------------
Perhaps add // FIXME: Explain! where we'll need lots more comments so users can understand this (mainly for the interface we expect users to code checks against).

================
Comment at: clang-tidy/ClangTidy.h:72
@@ +71,3 @@
+/// \brief Handle detected clang-tidy checks.
+void handleErrors(ClangTidyErrors &Errors, bool Fix);
+
----------------
What does !Fix do? Perhaps use an enum?

================
Comment at: clang-tidy/ClangTidy.cpp:54
@@ +53,3 @@
+    Checks("checks",
+           cl::desc("Regular expression matching the checks to be run."),
+           cl::init(".*"), cl::cat(ClangTidyCategory));
----------------
Perhaps ... names of the checks ...?

================
Comment at: clang-tidy/ClangTidy.cpp:58
@@ +57,3 @@
+                         cl::init(false), cl::cat(ClangTidyCategory));
+
+namespace clang {
----------------
Do we want a way to list all possible checks?

================
Comment at: clang-tidy/ClangTidyModule.h:21
@@ +20,3 @@
+
+typedef std::pair<StringRef, ClangTidyCheck *> NamedCheck;
+typedef SmallVector<NamedCheck, 16> ClangTidyCheckVector;
----------------
I'd probably use a getName() in ClangTidyCheck instead.

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

================
Comment at: clang-tidy/ClangTidyModule.h:29
@@ +28,3 @@
+  virtual ~ClangTidyModule() {}
+  virtual void addChecks(ClangTidyCheckVector *Checks) = 0;
+};
----------------
I don't think the typedef buys us much here, but I might be alone with my opinion...

================
Comment at: clang-tidy/ClangTidyModuleRegistry.h:20
@@ +19,3 @@
+typedef llvm::Registry<ClangTidyModule>
+    ClangTidyModuleRegistry;
+
----------------
Again, a typedef I probably wouldn't have used...

================
Comment at: clang-tidy/llvm/LLVMModule.cpp:29
@@ +28,3 @@
+
+class NamespaceCallback : public MatchFinder::MatchCallback {
+public:
----------------
Perhaps call it NamespaceCommentCallback so it's clear what it checks. Or do you expect we want multiple checks for namespaces in one of those callbacks?


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