[PATCH] Initial clang-tidy architecture

[Daniel Jasper]

================
Comment at: clang-tidy/ClangTidy.h:27
@@ +26,3 @@
+struct ClangTidyError {
+  ClangTidyError(const SourceManager &Sources, SourceLocation Loc,
+                 StringRef Message, const tooling::Replacements &Fix)
----------------
Manuel Klimek wrote:
> Not sure, but we might want a range.
I think this will be used for two different things:
a) Display diagnostics. I can't come up with all that many checks that would need a range. Moreover, some of the ranges (e.g. the namespace-comment-check) might the involve a multi-line range, which can't be nicely displayed either way.
b) Selecting whether an error is relevant. The user specifies one or more ranges that should be tidy-checked. Now, it is quite clear what it means when an error occurs on a specific location (we just need to check whether it is in any of those ranges). If we would let the user specify a range, this would become less clear. E.g. assume for a missing namespace comment, we would create the range for the entire namespace. Do we then want to do the check / report the result whenever we run clang-tidy on anything within said namespace? Or only if the start or end of the range are in one of the checked ranges?

I think for now, an interface with just a location is easier and more clearly defined. Thus, I'd like to stick with it for now. I have made the members of ClangTidyError private. That way, we don't need to change the Check-interface if we extend to give a range-option.

================
Comment at: clang-tidy/ClangTidy.h:42
@@ +41,3 @@
+
+/// \brief Every \c ClangTidyCheck reports errors through a conetxt.
+class ClangTidyContext {
----------------
João Matos wrote:
> Manuel Klimek wrote:
> > s/conetxt/context/
> typo
Fixed.

================
Comment at: clang-tidy/llvm/LLVMModule.cpp:29
@@ +28,3 @@
+
+class NamespaceCallback : public MatchFinder::MatchCallback {
+public:
----------------
Manuel Klimek wrote:
> 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?
Done. This was just left over back from the time where it was a NamespaceCheck ;-)

================
Comment at: clang-tidy/ClangTidyModuleRegistry.h:20
@@ +19,3 @@
+typedef llvm::Registry<ClangTidyModule>
+    ClangTidyModuleRegistry;
+
----------------
Manuel Klimek wrote:
> Again, a typedef I probably wouldn't have used...
I have found this pattern with other llvm::Registry usages, so I copied it. Not sure whether it adds much. The main advantage I can see is that now you can actually include this header file and see the name of the Registry etc. without that, where would you put this information. But again, my main point is consistency with other usages.

================
Comment at: clang-tidy/ClangTidyModule.h:29
@@ +28,3 @@
+  virtual ~ClangTidyModule() {}
+  virtual void addChecks(ClangTidyCheckVector *Checks) = 0;
+};
----------------
Manuel Klimek wrote:
> I don't think the typedef buys us much here, but I might be alone with my opinion...
As you have just asked my to change it above and that change would already have required me to change more than a handful of instances, I think you have proven yourself wrong ;-).

================
Comment at: clang-tidy/ClangTidyModule.h:21
@@ +20,3 @@
+
+typedef std::pair<StringRef, ClangTidyCheck *> NamedCheck;
+typedef SmallVector<NamedCheck, 16> ClangTidyCheckVector;
----------------
Manuel Klimek wrote:
> I'd probably use a getName() in ClangTidyCheck instead.
The point is that I want this to be a configuration feature of the Module instead of an implementation detail of the check. I think we might arrive at a point where the same Check-class is used with slightly different options in several modules and thereby with different names.

================
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));
----------------
Manuel Klimek wrote:
> Perhaps ... names of the checks ...?
Changed.

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

================
Comment at: clang-tidy/ClangTidy.h:40
@@ +39,3 @@
+
+typedef SmallVector<ClangTidyError, 16> ClangTidyErrors;
+
----------------
Manuel Klimek wrote:
> Use SmallVectorImpl?
The problem here is that this is returned from runClangTidy() and thus it outlives the ClangTidyContext. Thus, we need to copy it at the moment. However, similar to tooling::Replacements, these are small and lightweight objects, so copying them should not be harmful.

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

================
Comment at: clang-tidy/ClangTidy.h:72
@@ +71,3 @@
+/// \brief Handle detected clang-tidy checks.
+void handleErrors(ClangTidyErrors &Errors, bool Fix);
+
----------------
Manuel Klimek wrote:
> What does !Fix do? Perhaps use an enum?
Updated comment. I think this is a temporary interface anyway. We'll want to have something like "confidence" levels and only display/fix errors exceeding a specific level. For this patch, I'd like to keep this as simple as possible.

================
Comment at: clang-tidy/ClangTidy.h:45
@@ +44,3 @@
+public:
+  void reportError(const ClangTidyError &Error) { Errors.push_back(Error); }
+
----------------
Manuel Klimek wrote:
> 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).
Done.

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


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