Add the check name to the clang-tidy diagnostic output.

Summary:
Pass check names all the way from ClangTidyModule through
ClangTidyCheck and ClangTidyContext to ClangTidyError, and output it in
handleErrors. This allows to find mis-behaving check and disable it easily.

Reviewers: djasper, klimek

Reviewed By: djasper

CC: cfe-commits

Differential Revision: http://llvm-reviews.chandlerc.com/D2534

llvm-svn: 199094

clang-tools-extra/clang-tidy/ClangTidy.cpp

@@ -174,11 +174,20 @@ bool ChecksFilter::IsCheckEnabled(StringRef Name) {
   return EnableChecks.match(Name) && !DisableChecks.match(Name);
 }
 
+DiagnosticBuilder ClangTidyCheck::diag(SourceLocation Loc, StringRef Message) {
+  return Context->diag(CheckName, Loc, Message);
+}
+
 void ClangTidyCheck::run(const ast_matchers::MatchFinder::MatchResult &Result) {
   Context->setSourceManager(Result.SourceManager);
   check(Result);
 }
 
+void ClangTidyCheck::setName(StringRef Name) {
+  assert(CheckName.empty());
+  CheckName = Name.str();
+}
+
 std::vector<std::string> getCheckNames(StringRef EnableChecksRegex,
                                        StringRef DisableChecksRegex) {
   SmallVector<ClangTidyError, 8> Errors;
@@ -231,7 +240,8 @@ void runClangTidy(StringRef EnableChecksRegex, StringRef DisableChecksRegex,
 static void reportDiagnostic(const ClangTidyMessage &Message,
                              SourceManager &SourceMgr,
                              DiagnosticsEngine::Level Level,
-                             DiagnosticsEngine &Diags) {
+                             DiagnosticsEngine &Diags,
+                             StringRef CheckName = "") {
   SourceLocation Loc;
   if (!Message.FilePath.empty()) {
     const FileEntry *File =
@@ -240,7 +250,11 @@ static void reportDiagnostic(const ClangTidyMessage &Message,
     Loc = SourceMgr.getLocForStartOfFile(ID);
     Loc = Loc.getLocWithOffset(Message.FileOffset);
   }
-  Diags.Report(Loc, Diags.getCustomDiagID(Level, Message.Message));
+  if (CheckName.empty())
+    Diags.Report(Loc, Diags.getCustomDiagID(Level, Message.Message));
+  else
+    Diags.Report(Loc, Diags.getCustomDiagID(Level, (Message.Message + " [" +
+                                                    CheckName + "]").str()));
 }
 
 void handleErrors(SmallVectorImpl<ClangTidyError> &Errors, bool Fix) {
@@ -256,7 +270,8 @@ void handleErrors(SmallVectorImpl<ClangTidyError> &Errors, bool Fix) {
   for (SmallVectorImpl<ClangTidyError>::iterator I = Errors.begin(),
                                                  E = Errors.end();
        I != E; ++I) {
-    reportDiagnostic(I->Message, SourceMgr, DiagnosticsEngine::Warning, Diags);
+    reportDiagnostic(I->Message, SourceMgr, DiagnosticsEngine::Warning, Diags,
+                     I->CheckName);
     for (unsigned i = 0, e = I->Notes.size(); i != e; ++i) {
       reportDiagnostic(I->Notes[i], SourceMgr, DiagnosticsEngine::Note, Diags);
     }

clang-tools-extra/clang-tidy/ClangTidy.h

@@ -75,11 +75,17 @@ public:
   /// \brief The infrastructure sets the context to \p Ctx with this function.
   void setContext(ClangTidyContext *Ctx) { Context = Ctx; }
 
-protected:
-  ClangTidyContext *Context;
+  /// \brief Add a diagnostic with the check's name.
+  DiagnosticBuilder diag(SourceLocation Loc, StringRef Message);
+
+  /// \brief Sets the check name. Intended to be used by the clang-tidy
+  /// framework. Can be called only once.
+  void setName(StringRef Name);
 
 private:
   virtual void run(const ast_matchers::MatchFinder::MatchResult &Result);
+  ClangTidyContext *Context;
+  std::string CheckName;
 };
 
 /// \brief Filters checks by name.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

@@ -33,13 +33,18 @@ ClangTidyMessage::ClangTidyMessage(StringRef Message,
   FileOffset = Sources.getFileOffset(Loc);
 }
 
-ClangTidyError::ClangTidyError(const ClangTidyMessage &Message)
-    : Message(Message) {}
+ClangTidyError::ClangTidyError(StringRef CheckName,
+                               const ClangTidyMessage &Message)
+    : CheckName(CheckName), Message(Message) {}
 
-DiagnosticBuilder ClangTidyContext::Diag(SourceLocation Loc,
+DiagnosticBuilder ClangTidyContext::diag(StringRef CheckName,
+                                         SourceLocation Loc,
                                          StringRef Message) {
-  return DiagEngine->Report(
-      Loc, DiagEngine->getCustomDiagID(DiagnosticsEngine::Warning, Message));
+  unsigned ID =
+      DiagEngine->getCustomDiagID(DiagnosticsEngine::Warning, Message);
+  if (CheckNamesByDiagnosticID.count(ID) == 0)
+    CheckNamesByDiagnosticID.insert(std::make_pair(ID, CheckName.str()));
+  return DiagEngine->Report(Loc, ID);
 }
 
 void ClangTidyContext::setDiagnosticsEngine(DiagnosticsEngine *Engine) {
@@ -55,6 +60,14 @@ void ClangTidyContext::storeError(const ClangTidyError &Error) {
   Errors->push_back(Error);
 }
 
+StringRef ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
+  llvm::DenseMap<unsigned, std::string>::const_iterator I =
+      CheckNamesByDiagnosticID.find(DiagnosticID);
+  if (I != CheckNamesByDiagnosticID.end())
+    return I->second;
+  return "";
+}
+
 ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(ClangTidyContext &Ctx)
     : Context(Ctx) {
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
@@ -72,7 +85,8 @@ void ClangTidyDiagnosticConsumer::HandleDiagnostic(
   if (Diags->getSourceManager().isInSystemHeader(Info.getLocation()))
     return;
   if (DiagLevel != DiagnosticsEngine::Note) {
-    Errors.push_back(ClangTidyError(getMessage(Info)));
+    Errors.push_back(
+        ClangTidyError(Context.getCheckName(Info.getID()), getMessage(Info)));
   } else {
     assert(!Errors.empty() &&
            "A diagnostic note can only be appended to a message.");

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

@@ -13,6 +13,7 @@
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Refactoring.h"
+#include "llvm/ADT/DenseMap.h"
 
 namespace clang {
 
@@ -46,8 +47,9 @@ struct ClangTidyMessage {
 ///
 /// FIXME: Make Diagnostics flexible enough to support this directly.
 struct ClangTidyError {
-  ClangTidyError(const ClangTidyMessage &Message);
+  ClangTidyError(StringRef CheckName, const ClangTidyMessage &Message);
 
+  std::string CheckName;
   ClangTidyMessage Message;
   tooling::Replacements Fix;
   SmallVector<ClangTidyMessage, 1> Notes;
@@ -72,7 +74,8 @@ public:
   /// This is still under heavy development and will likely change towards using
   /// tablegen'd diagnostic IDs.
   /// FIXME: Figure out a way to manage ID spaces.
-  DiagnosticBuilder Diag(SourceLocation Loc, StringRef Message);
+  DiagnosticBuilder diag(StringRef CheckName, SourceLocation Loc,
+                         StringRef Message);
 
   /// \brief Sets the \c DiagnosticsEngine so that Diagnostics can be generated
   /// correctly.
@@ -85,6 +88,10 @@ public:
   /// This is called from the \c ClangTidyCheck base class.
   void setSourceManager(SourceManager *SourceMgr);
 
+  /// \brief Returns the name of the clang-tidy check which produced this
+  /// diagnostic ID.
+  StringRef getCheckName(unsigned DiagnosticID) const;
+
 private:
   friend class ClangTidyDiagnosticConsumer; // Calls storeError().
 
@@ -93,6 +100,7 @@ private:
 
   SmallVectorImpl<ClangTidyError> *Errors;
   DiagnosticsEngine *DiagEngine;
+  llvm::DenseMap<unsigned, std::string> CheckNamesByDiagnosticID;
 };
 
 /// \brief A diagnostic consumer that turns each \c Diagnostic into a

clang-tools-extra/clang-tidy/ClangTidyModule.cpp

@@ -32,8 +32,11 @@ void ClangTidyCheckFactories::createChecks(
     ChecksFilter &Filter, SmallVectorImpl<ClangTidyCheck *> &Checks) {
   for (FactoryMap::iterator I = Factories.begin(), E = Factories.end(); I != E;
        ++I) {
-    if (Filter.IsCheckEnabled(I->first))
-      Checks.push_back(I->second->createCheck());
+    if (Filter.IsCheckEnabled(I->first)) {
+      ClangTidyCheck *Check = I->second->createCheck();
+      Check->setName(I->first);
+      Checks.push_back(Check);
+    }
   }
 }
 

clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp

@@ -35,7 +35,7 @@ void ExplicitConstructorCheck::check(const MatchFinder::MatchResult &Result) {
   if (!Ctor->isExplicit() && !Ctor->isImplicit() && Ctor->getNumParams() >= 1 &&
       Ctor->getMinRequiredArguments() <= 1) {
     SourceLocation Loc = Ctor->getLocation();
-    Context->Diag(Loc, "Single-argument constructors must be explicit")
+    diag(Loc, "Single-argument constructors must be explicit")
         << FixItHint::CreateInsertion(Loc, "explicit ");
   }
 }

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

@@ -40,14 +40,14 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
   // FIXME: Check that this namespace is "long".
   if (Tok.is(tok::comment)) {
     // FIXME: Check comment content.
+    // FIXME: Check comment placement on the same line.
     return;
   }
   std::string Fix = " // namespace";
   if (!ND->isAnonymousNamespace())
     Fix = Fix.append(" ").append(ND->getNameAsString());
 
-  Context->Diag(ND->getLocation(),
-                "namespace not terminated with a closing comment")
+  diag(ND->getLocation(), "namespace not terminated with a closing comment")
       << FixItHint::CreateInsertion(ND->getRBraceLoc().getLocWithOffset(1),
                                     Fix);
 }
@@ -55,8 +55,8 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
 namespace {
 class IncludeOrderPPCallbacks : public PPCallbacks {
 public:
-  explicit IncludeOrderPPCallbacks(ClangTidyContext &Context)
-      : Context(Context) {}
+  explicit IncludeOrderPPCallbacks(IncludeOrderCheck &Check)
+      : Check(Check) {}
 
   virtual void InclusionDirective(SourceLocation HashLoc,
                                   const Token &IncludeTok, StringRef FileName,
@@ -66,17 +66,17 @@ public:
                                   const Module *Imported) {
     // FIXME: This is a dummy implementation to show how to get at preprocessor
     // information. Implement a real include order check.
-    Context.Diag(HashLoc, "This is an include");
+    Check.diag(HashLoc, "This is an include");
   }
 
 private:
-  ClangTidyContext &Context;
+  IncludeOrderCheck &Check;
 };
 } // namespace
 
 void IncludeOrderCheck::registerPPCallbacks(CompilerInstance &Compiler) {
   Compiler.getPreprocessor()
-      .addPPCallbacks(new IncludeOrderPPCallbacks(*Context));
+      .addPPCallbacks(new IncludeOrderPPCallbacks(*this));
 }
 
 class LLVMModule : public ClangTidyModule {

clang-tools-extra/test/clang-tidy/basic.cpp

@@ -4,4 +4,4 @@
 
 namespace i {
 }
-// CHECK: warning: namespace not terminated with a closing comment
+// CHECK: warning: namespace not terminated with a closing comment [llvm-namespace-comment]