From 41bfe8dde13b629a13e95e3b5f31ba1efca993ac Mon Sep 17 00:00:00 2001 From: Alexander Kornienko Date: Mon, 13 Jan 2014 10:50:51 +0000 Subject: [PATCH] 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 | 21 ++++++++++++--- clang-tools-extra/clang-tidy/ClangTidy.h | 10 +++++-- .../ClangTidyDiagnosticConsumer.cpp | 26 ++++++++++++++----- .../clang-tidy/ClangTidyDiagnosticConsumer.h | 12 +++++++-- .../clang-tidy/ClangTidyModule.cpp | 7 +++-- .../clang-tidy/google/GoogleTidyModule.cpp | 2 +- .../clang-tidy/llvm/LLVMTidyModule.cpp | 14 +++++----- clang-tools-extra/test/clang-tidy/basic.cpp | 2 +- 8 files changed, 70 insertions(+), 24 deletions(-) diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp index bbcd0204f2df..c17b92498471 100644 --- a/clang-tools-extra/clang-tidy/ClangTidy.cpp +++ b/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 getCheckNames(StringRef EnableChecksRegex, StringRef DisableChecksRegex) { SmallVector 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 &Errors, bool Fix) { @@ -256,7 +270,8 @@ void handleErrors(SmallVectorImpl &Errors, bool Fix) { for (SmallVectorImpl::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); } diff --git a/clang-tools-extra/clang-tidy/ClangTidy.h b/clang-tools-extra/clang-tidy/ClangTidy.h index 16889bff80a9..f57f8f464c28 100644 --- a/clang-tools-extra/clang-tidy/ClangTidy.h +++ b/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. diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp index 4d92222588bb..bae52be3d035 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/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::const_iterator I = + CheckNamesByDiagnosticID.find(DiagnosticID); + if (I != CheckNamesByDiagnosticID.end()) + return I->second; + return ""; +} + ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(ClangTidyContext &Ctx) : Context(Ctx) { IntrusiveRefCntPtr 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."); diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h index f8ac6efdf312..f5e2857f66f5 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h +++ b/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 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 *Errors; DiagnosticsEngine *DiagEngine; + llvm::DenseMap CheckNamesByDiagnosticID; }; /// \brief A diagnostic consumer that turns each \c Diagnostic into a diff --git a/clang-tools-extra/clang-tidy/ClangTidyModule.cpp b/clang-tools-extra/clang-tidy/ClangTidyModule.cpp index dc3a11d0d545..87ab2be3c485 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyModule.cpp @@ -32,8 +32,11 @@ void ClangTidyCheckFactories::createChecks( ChecksFilter &Filter, SmallVectorImpl &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); + } } } diff --git a/clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp b/clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp index bf1e3e38a3ac..8b25d0292e0e 100644 --- a/clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp +++ b/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 "); } } diff --git a/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp b/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp index ec1e779a8e8a..137bb6c41ce6 100644 --- a/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp +++ b/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 { diff --git a/clang-tools-extra/test/clang-tidy/basic.cpp b/clang-tools-extra/test/clang-tidy/basic.cpp index e61b8ebfb138..87565c8dcfc8 100644 --- a/clang-tools-extra/test/clang-tidy/basic.cpp +++ b/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]