[clang-tools-extra] r361112 - [clangd] Respect clang-tidy suppression comments
Fangrui Song via cfe-commits
cfe-commits at lists.llvm.org
Sat May 18 21:06:52 PDT 2019
Author: maskray
Date: Sat May 18 21:06:52 2019
New Revision: 361112
URL: http://llvm.org/viewvc/llvm-project?rev=361112&view=rev
Log:
[clangd] Respect clang-tidy suppression comments
This partially fixes https://bugs.llvm.org/show_bug.cgi?id=41218.
Reviewed By: sammccall
Patch by Nathan Ridge!
Differential Revision: https://reviews.llvm.org/D60953
Modified:
clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tools-extra/trunk/clangd/ClangdUnit.cpp
clang-tools-extra/trunk/clangd/Diagnostics.cpp
clang-tools-extra/trunk/clangd/Diagnostics.h
clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp
Modified: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp?rev=361112&r1=361111&r2=361112&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp Sat May 18 21:06:52 2019
@@ -166,11 +166,13 @@ public:
bool contains(StringRef S) {
switch (auto &Result = Cache[S]) {
- case Yes: return true;
- case No: return false;
- case None:
- Result = Globs.contains(S) ? Yes : No;
- return Result == Yes;
+ case Yes:
+ return true;
+ case No:
+ return false;
+ case None:
+ Result = Globs.contains(S) ? Yes : No;
+ return Result == Yes;
}
llvm_unreachable("invalid enum");
}
@@ -387,16 +389,30 @@ static bool LineIsMarkedWithNOLINTinMacr
return false;
}
+namespace clang {
+namespace tidy {
+
+bool ShouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
+ const Diagnostic &Info, ClangTidyContext &Context,
+ bool CheckMacroExpansion) {
+ return Info.getLocation().isValid() &&
+ DiagLevel != DiagnosticsEngine::Error &&
+ DiagLevel != DiagnosticsEngine::Fatal &&
+ (CheckMacroExpansion ? LineIsMarkedWithNOLINTinMacro
+ : LineIsMarkedWithNOLINT)(Info.getSourceManager(),
+ Info.getLocation(),
+ Info.getID(), Context);
+}
+
+} // namespace tidy
+} // namespace clang
+
void ClangTidyDiagnosticConsumer::HandleDiagnostic(
DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info) {
if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
return;
- if (Info.getLocation().isValid() && DiagLevel != DiagnosticsEngine::Error &&
- DiagLevel != DiagnosticsEngine::Fatal &&
- LineIsMarkedWithNOLINTinMacro(Info.getSourceManager(),
- Info.getLocation(), Info.getID(),
- Context)) {
+ if (ShouldSuppressDiagnostic(DiagLevel, Info, Context)) {
++Context.Stats.ErrorsIgnoredNOLINT;
// Ignored a warning, should ignore related notes as well
LastErrorWasIgnored = true;
Modified: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h?rev=361112&r1=361111&r2=361112&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h (original)
+++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h Sat May 18 21:06:52 2019
@@ -217,6 +217,23 @@ private:
bool AllowEnablingAnalyzerAlphaCheckers;
};
+/// Check whether a given diagnostic should be suppressed due to the presence
+/// of a "NOLINT" suppression comment.
+/// This is exposed so that other tools that present clang-tidy diagnostics
+/// (such as clangd) can respect the same suppression rules as clang-tidy.
+/// This does not handle suppression of notes following a suppressed diagnostic;
+/// that is left to the caller is it requires maintaining state in between calls
+/// to this function.
+/// The `CheckMacroExpansion` parameter determines whether the function should
+/// handle the case where the diagnostic is inside a macro expansion. A degree
+/// of control over this is needed because handling this case can require
+/// examining source files other than the one in which the diagnostic is
+/// located, and in some use cases we cannot rely on such other files being
+/// mapped in the SourceMapper.
+bool ShouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
+ const Diagnostic &Info, ClangTidyContext &Context,
+ bool CheckMacroExpansion = true);
+
/// \brief A diagnostic consumer that turns each \c Diagnostic into a
/// \c SourceManager-independent \c ClangTidyError.
//
@@ -246,7 +263,7 @@ private:
/// \brief Updates \c LastErrorRelatesToUserCode and LastErrorPassesLineFilter
/// according to the diagnostic \p Location.
- void checkFilters(SourceLocation Location, const SourceManager& Sources);
+ void checkFilters(SourceLocation Location, const SourceManager &Sources);
bool passesLineFilter(StringRef FileName, unsigned LineNumber) const;
ClangTidyContext &Context;
Modified: clang-tools-extra/trunk/clangd/ClangdUnit.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdUnit.cpp?rev=361112&r1=361111&r2=361112&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdUnit.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdUnit.cpp Sat May 18 21:06:52 2019
@@ -113,12 +113,12 @@ public:
}
void EndOfMainFile() {
- for (const auto& Entry : MainFileMacros)
+ for (const auto &Entry : MainFileMacros)
Out->push_back(Entry.getKey());
llvm::sort(*Out);
}
- private:
+private:
const SourceManager &SM;
bool InMainFile = true;
llvm::StringSet<> MainFileMacros;
@@ -332,6 +332,30 @@ ParsedAST::build(std::unique_ptr<Compile
CTContext->setASTContext(&Clang->getASTContext());
CTContext->setCurrentFile(MainInput.getFile());
CTFactories.createChecks(CTContext.getPointer(), CTChecks);
+ ASTDiags.suppressDiagnostics([&CTContext](
+ DiagnosticsEngine::Level DiagLevel,
+ const clang::Diagnostic &Info) {
+ if (CTContext) {
+ bool IsClangTidyDiag = !CTContext->getCheckName(Info.getID()).empty();
+ if (IsClangTidyDiag) {
+ // Skip the ShouldSuppressDiagnostic check for diagnostics not in
+ // the main file, because we don't want that function to query the
+ // source buffer for preamble files. For the same reason, we ask
+ // ShouldSuppressDiagnostic not to follow macro expansions, since
+ // those might take us into a preamble file as well.
+ bool IsInsideMainFile =
+ Info.hasSourceManager() &&
+ Info.getSourceManager().isWrittenInMainFile(
+ Info.getSourceManager().getFileLoc(Info.getLocation()));
+ if (IsInsideMainFile && tidy::ShouldSuppressDiagnostic(
+ DiagLevel, Info, *CTContext,
+ /* CheckMacroExpansion = */ false)) {
+ return true;
+ }
+ }
+ }
+ return false;
+ });
Preprocessor *PP = &Clang->getPreprocessor();
for (const auto &Check : CTChecks) {
// FIXME: the PP callbacks skip the entire preamble.
Modified: clang-tools-extra/trunk/clangd/Diagnostics.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Diagnostics.cpp?rev=361112&r1=361111&r2=361112&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Diagnostics.cpp (original)
+++ clang-tools-extra/trunk/clangd/Diagnostics.cpp Sat May 18 21:06:52 2019
@@ -514,6 +514,12 @@ void StoreDiags::HandleDiagnostic(Diagno
// Handle the new main diagnostic.
flushLastDiag();
+ if (SuppressionFilter && SuppressionFilter(DiagLevel, Info)) {
+ LastPrimaryDiagnosticWasSuppressed = true;
+ return;
+ }
+ LastPrimaryDiagnosticWasSuppressed = false;
+
LastDiag = Diag();
LastDiag->ID = Info.getID();
FillDiagBase(*LastDiag);
@@ -528,6 +534,13 @@ void StoreDiags::HandleDiagnostic(Diagno
}
} else {
// Handle a note to an existing diagnostic.
+
+ // If a diagnostic was suppressed due to the suppression filter,
+ // also suppress notes associated with it.
+ if (LastPrimaryDiagnosticWasSuppressed) {
+ return;
+ }
+
if (!LastDiag) {
assert(false && "Adding a note without main diagnostic");
IgnoreDiagnostics::log(DiagLevel, Info);
Modified: clang-tools-extra/trunk/clangd/Diagnostics.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Diagnostics.h?rev=361112&r1=361111&r2=361112&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Diagnostics.h (original)
+++ clang-tools-extra/trunk/clangd/Diagnostics.h Sat May 18 21:06:52 2019
@@ -128,17 +128,25 @@ public:
using DiagFixer = std::function<std::vector<Fix>(DiagnosticsEngine::Level,
const clang::Diagnostic &)>;
+ using DiagFilter =
+ std::function<bool(DiagnosticsEngine::Level, const clang::Diagnostic &)>;
/// If set, possibly adds fixes for diagnostics using \p Fixer.
void contributeFixes(DiagFixer Fixer) { this->Fixer = Fixer; }
+ /// If set, ignore diagnostics for which \p SuppressionFilter returns true.
+ void suppressDiagnostics(DiagFilter SuppressionFilter) {
+ this->SuppressionFilter = SuppressionFilter;
+ }
private:
void flushLastDiag();
DiagFixer Fixer = nullptr;
+ DiagFilter SuppressionFilter = nullptr;
std::vector<Diag> Output;
llvm::Optional<LangOptions> LangOpts;
llvm::Optional<Diag> LastDiag;
llvm::DenseSet<int> IncludeLinesWithErrors;
+ bool LastPrimaryDiagnosticWasSuppressed = false;
};
} // namespace clangd
Modified: clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp?rev=361112&r1=361111&r2=361112&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp (original)
+++ clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp Sat May 18 21:06:52 2019
@@ -207,6 +207,26 @@ TEST(DiagnosticsTest, ClangTidy) {
"multiple unsequenced modifications to 'y'")));
}
+TEST(DiagnosticTest, ClangTidySuppressionComment) {
+ Annotations Main(R"cpp(
+ int main() {
+ int i = 3;
+ double d = 8 / i; // NOLINT
+ // NOLINTNEXTLINE
+ double e = 8 / i;
+ double f = [[8]] / i;
+ }
+ )cpp");
+ TestTU TU = TestTU::withCode(Main.code());
+ TU.ClangTidyChecks = "bugprone-integer-division";
+ EXPECT_THAT(
+ TU.build().getDiagnostics(),
+ UnorderedElementsAre(::testing::AllOf(
+ Diag(Main.range(), "result of integer division used in a floating "
+ "point context; possible loss of precision"),
+ DiagSource(Diag::ClangTidy), DiagName("bugprone-integer-division"))));
+}
+
TEST(DiagnosticsTest, Preprocessor) {
// This looks like a preamble, but there's an #else in the middle!
// Check that:
@@ -767,6 +787,7 @@ TEST(DiagsInHeaders, OnlyErrorOrFatal) {
"a type specifier for all declarations"),
WithNote(Diag(Header.range(), "error occurred here")))));
}
+
} // namespace
} // namespace clangd
More information about the cfe-commits
mailing list