kadircet created this revision.
kadircet added a reviewer: ilya-biryukov.
Herald added subscribers: usaxena95, arphaman.
Herald added a project: All.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang-tools-extra.
Fixes https://github.com/clangd/clangd/issues/1142
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D127844
Files:
clang-tools-extra/clangd/Diagnostics.cpp
clang-tools-extra/clangd/Diagnostics.h
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/unittests/CompilerTests.cpp
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1870,6 +1870,27 @@
"'int' to 'int *' for 1st argument; take the address of "
"the argument with &")))));
}
+
+TEST(DiagnosticsTest, DontSuppressSubcategories) {
+ Annotations Source(R"cpp(
+ /*error-ok*/
+ void bar(int x) {
+ switch(x) {
+ default:
+ break;
+ break;
+ }
+ })cpp");
+ TestTU TU;
+ TU.ExtraArgs.push_back("-Wunreachable-code-aggressive");
+ TU.Code = Source.code().str();
+ Config Cfg;
+ // This shouldn't suppress subcategory unreachable-break.
+ Cfg.Diagnostics.Suppress = {"unreachable-code"};
+ WithContextValue SuppressFilterWithCfg(Config::Key, std::move(Cfg));
+ EXPECT_THAT(*TU.build().getDiagnostics(),
+ ElementsAre(diagName("-Wunreachable-code-break")));
+}
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -284,6 +284,7 @@
}
TEST_F(ConfigCompileTests, DiagnosticSuppression) {
+ // Check that canonicalization and compilation works.
Frag.Diagnostics.Suppress.emplace_back("bugprone-use-after-move");
Frag.Diagnostics.Suppress.emplace_back("unreachable-code");
Frag.Diagnostics.Suppress.emplace_back("-Wunused-variable");
@@ -296,21 +297,8 @@
"unreachable-code", "unused-variable",
"typecheck_bool_condition",
"unexpected_friend", "warn_alloca"));
- EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
- diag::warn_unreachable, Conf.Diagnostics.Suppress, LangOptions()));
- // Subcategory not respected/suppressed.
- EXPECT_FALSE(isBuiltinDiagnosticSuppressed(
- diag::warn_unreachable_break, Conf.Diagnostics.Suppress, LangOptions()));
- EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
- diag::warn_unused_variable, Conf.Diagnostics.Suppress, LangOptions()));
- EXPECT_TRUE(isBuiltinDiagnosticSuppressed(diag::err_typecheck_bool_condition,
- Conf.Diagnostics.Suppress,
- LangOptions()));
- EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
- diag::err_unexpected_friend, Conf.Diagnostics.Suppress, LangOptions()));
- EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
- diag::warn_alloca, Conf.Diagnostics.Suppress, LangOptions()));
+ // Check that we treat * specially.
Frag.Diagnostics.Suppress.emplace_back("*");
EXPECT_TRUE(compileAndApply());
EXPECT_TRUE(Conf.Diagnostics.SuppressAll);
Index: clang-tools-extra/clangd/unittests/CompilerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/CompilerTests.cpp
+++ clang-tools-extra/clangd/unittests/CompilerTests.cpp
@@ -7,6 +7,8 @@
//===----------------------------------------------------------------------===//
#include "Compiler.h"
+#include "Config.h"
+#include "Diagnostics.h"
#include "TestTU.h"
#include "clang/Frontend/DependencyOutputOptions.h"
#include "clang/Frontend/FrontendOptions.h"
@@ -113,6 +115,20 @@
// No crash.
EXPECT_EQ(buildCompilerInvocation(Inputs, Diags), nullptr);
}
+
+TEST(BuildCompilerInvocation, SuppressDiags) {
+ MockFS FS;
+ StoreDiags Diags;
+ TestTU TU;
+ TU.ExtraArgs = {"-funknown-arg"};
+ auto Inputs = TU.inputs(FS);
+
+ Config Cfg;
+ Cfg.Diagnostics.Suppress = {"drv_unknown_argument"};
+ WithContextValue SuppressFilterWithCfg(Config::Key, std::move(Cfg));
+ EXPECT_NE(buildCompilerInvocation(Inputs, Diags), nullptr);
+ EXPECT_THAT(Diags.take(), IsEmpty());
+}
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/Preamble.cpp
===================================================================
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -491,25 +491,20 @@
llvm::IntrusiveRefCntPtr<DiagnosticsEngine> PreambleDiagsEngine =
CompilerInstance::createDiagnostics(&CI.getDiagnosticOpts(),
&PreambleDiagnostics, false);
- const Config &Cfg = Config::current();
- PreambleDiagnostics.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
- const clang::Diagnostic &Info) {
- if (Cfg.Diagnostics.SuppressAll ||
- isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress,
- *CI.getLangOpts()))
- return DiagnosticsEngine::Ignored;
- switch (Info.getID()) {
- case diag::warn_no_newline_eof:
- case diag::warn_cxx98_compat_no_newline_eof:
- case diag::ext_no_newline_eof:
- // If the preamble doesn't span the whole file, drop the no newline at
- // eof warnings.
- return Bounds.Size != ContentsBuffer->getBufferSize()
- ? DiagnosticsEngine::Level::Ignored
- : DiagLevel;
- }
- return DiagLevel;
- });
+ PreambleDiagnostics.setLevelAdjuster(
+ [&](DiagnosticsEngine::Level DiagLevel, const clang::Diagnostic &Info) {
+ switch (Info.getID()) {
+ case diag::warn_no_newline_eof:
+ case diag::warn_cxx98_compat_no_newline_eof:
+ case diag::ext_no_newline_eof:
+ // If the preamble doesn't span the whole file, drop the no newline at
+ // eof warnings.
+ return Bounds.Size != ContentsBuffer->getBufferSize()
+ ? DiagnosticsEngine::Level::Ignored
+ : DiagLevel;
+ }
+ return DiagLevel;
+ });
// Skip function bodies when building the preamble to speed up building
// the preamble and make it smaller.
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -502,51 +502,46 @@
}
const Config &Cfg = Config::current();
- ASTDiags.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
- const clang::Diagnostic &Info) {
- if (Cfg.Diagnostics.SuppressAll ||
- isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress,
- Clang->getLangOpts()))
- return DiagnosticsEngine::Ignored;
-
- auto It = OverriddenSeverity.find(Info.getID());
- if (It != OverriddenSeverity.end())
- DiagLevel = It->second;
-
- if (!CTChecks.empty()) {
- std::string CheckName = CTContext->getCheckName(Info.getID());
- bool IsClangTidyDiag = !CheckName.empty();
- if (IsClangTidyDiag) {
- if (Cfg.Diagnostics.Suppress.contains(CheckName))
- return DiagnosticsEngine::Ignored;
- // Check for suppression comment. Skip the 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 to avoid I/O.
- // We let suppression comments take precedence over warning-as-error
- // to match clang-tidy's behaviour.
- bool IsInsideMainFile =
- Info.hasSourceManager() &&
- isInsideMainFile(Info.getLocation(), Info.getSourceManager());
- SmallVector<tooling::Diagnostic, 1> TidySuppressedErrors;
- if (IsInsideMainFile && CTContext->shouldSuppressDiagnostic(
- DiagLevel, Info, TidySuppressedErrors,
- /*AllowIO=*/false,
- /*EnableNolintBlocks=*/true)) {
- // FIXME: should we expose the suppression error (invalid use of
- // NOLINT comments)?
- return DiagnosticsEngine::Ignored;
+ ASTDiags.setLevelAdjuster(
+ [&](DiagnosticsEngine::Level DiagLevel, const clang::Diagnostic &Info) {
+ auto It = OverriddenSeverity.find(Info.getID());
+ if (It != OverriddenSeverity.end())
+ DiagLevel = It->second;
+
+ if (!CTChecks.empty()) {
+ std::string CheckName = CTContext->getCheckName(Info.getID());
+ bool IsClangTidyDiag = !CheckName.empty();
+ if (IsClangTidyDiag) {
+ if (Cfg.Diagnostics.Suppress.contains(CheckName))
+ return DiagnosticsEngine::Ignored;
+ // Check for suppression comment. Skip the 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 to avoid I/O. We let
+ // suppression comments take precedence over warning-as-error to
+ // match clang-tidy's behaviour.
+ bool IsInsideMainFile =
+ Info.hasSourceManager() &&
+ isInsideMainFile(Info.getLocation(), Info.getSourceManager());
+ SmallVector<tooling::Diagnostic, 1> TidySuppressedErrors;
+ if (IsInsideMainFile && CTContext->shouldSuppressDiagnostic(
+ DiagLevel, Info, TidySuppressedErrors,
+ /*AllowIO=*/false,
+ /*EnableNolintBlocks=*/true)) {
+ // FIXME: should we expose the suppression error (invalid use of
+ // NOLINT comments)?
+ return DiagnosticsEngine::Ignored;
+ }
+
+ // Check for warning-as-error.
+ if (DiagLevel == DiagnosticsEngine::Warning &&
+ CTContext->treatAsError(CheckName)) {
+ return DiagnosticsEngine::Error;
+ }
+ }
}
-
- // Check for warning-as-error.
- if (DiagLevel == DiagnosticsEngine::Warning &&
- CTContext->treatAsError(CheckName)) {
- return DiagnosticsEngine::Error;
- }
- }
- }
- return DiagLevel;
- });
+ return DiagLevel;
+ });
// Add IncludeFixer which can recover diagnostics caused by missing includes
// (e.g. incomplete type) and attach include insertion fixes to diagnostics.
Index: clang-tools-extra/clangd/Diagnostics.h
===================================================================
--- clang-tools-extra/clangd/Diagnostics.h
+++ clang-tools-extra/clangd/Diagnostics.h
@@ -179,10 +179,6 @@
llvm::DenseSet<std::pair<unsigned, unsigned>> IncludedErrorLocations;
};
-/// Determine whether a (non-clang-tidy) diagnostic is suppressed by config.
-bool isBuiltinDiagnosticSuppressed(unsigned ID,
- const llvm::StringSet<> &Suppressed,
- const LangOptions &);
/// Take a user-specified diagnostic code, and convert it to a normalized form
/// stored in the config and consumed by isBuiltinDiagnosticsSuppressed.
///
Index: clang-tools-extra/clangd/Diagnostics.cpp
===================================================================
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -9,6 +9,7 @@
#include "Diagnostics.h"
#include "../clang-tidy/ClangTidyDiagnosticConsumer.h"
#include "Compiler.h"
+#include "Config.h"
#include "Protocol.h"
#include "SourceCode.h"
#include "support/Logger.h"
@@ -377,6 +378,26 @@
}
// FIXME: Set tags for tidy-based diagnostics too.
}
+
+// Determine whether a (non-clang-tidy) diagnostic is suppressed by config.
+bool isBuiltinDiagnosticSuppressed(unsigned ID,
+ const llvm::StringSet<> &Suppress,
+ const LangOptions *LO) {
+ // Don't complain about header-only stuff in mainfiles if it's a header.
+ // FIXME: would be cleaner to suppress in clang, once we decide whether the
+ // behavior should be to silently-ignore or respect the pragma.
+ if (LO && ID == diag::pp_pragma_sysheader_in_main_file && LO->IsHeaderFile)
+ return true;
+
+ if (const char *CodePtr = getDiagnosticCode(ID)) {
+ if (Suppress.contains(normalizeSuppressedCode(CodePtr)))
+ return true;
+ }
+ StringRef Warning = DiagnosticIDs::getWarningOptionForDiag(ID);
+ if (!Warning.empty() && Suppress.contains(Warning))
+ return true;
+ return false;
+}
} // namespace
llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const DiagBase &D) {
@@ -679,6 +700,20 @@
bool OriginallyError =
Info.getDiags()->getDiagnosticIDs()->isDefaultMappingAsError(
Info.getID());
+ if (!isNote(DiagLevel)) {
+ const Config &Cfg = Config::current();
+ // Check if diagnostics is suppressed (possibly by user), before doing any
+ // adjustments.
+ if (Cfg.Diagnostics.SuppressAll ||
+ isBuiltinDiagnosticSuppressed(
+ Info.getID(), Cfg.Diagnostics.Suppress,
+ LangOpts.hasValue() ? LangOpts.getPointer() : nullptr)) {
+ DiagLevel = DiagnosticsEngine::Ignored;
+ } else if (Adjuster) {
+ // FIXME: Merge with feature modules.
+ DiagLevel = Adjuster(DiagLevel, Info);
+ }
+ }
if (Info.getLocation().isInvalid()) {
// Handle diagnostics coming from command-line arguments. The source manager
@@ -793,9 +828,6 @@
flushLastDiag();
LastDiag = Diag();
- // FIXME: Merge with feature modules.
- if (Adjuster)
- DiagLevel = Adjuster(DiagLevel, Info);
FillDiagBase(*LastDiag);
if (isExcluded(LastDiag->ID))
@@ -882,25 +914,6 @@
Output.push_back(std::move(*LastDiag));
}
-bool isBuiltinDiagnosticSuppressed(unsigned ID,
- const llvm::StringSet<> &Suppress,
- const LangOptions &LangOpts) {
- // Don't complain about header-only stuff in mainfiles if it's a header.
- // FIXME: would be cleaner to suppress in clang, once we decide whether the
- // behavior should be to silently-ignore or respect the pragma.
- if (ID == diag::pp_pragma_sysheader_in_main_file && LangOpts.IsHeaderFile)
- return true;
-
- if (const char *CodePtr = getDiagnosticCode(ID)) {
- if (Suppress.contains(normalizeSuppressedCode(CodePtr)))
- return true;
- }
- StringRef Warning = DiagnosticIDs::getWarningOptionForDiag(ID);
- if (!Warning.empty() && Suppress.contains(Warning))
- return true;
- return false;
-}
-
llvm::StringRef normalizeSuppressedCode(llvm::StringRef Code) {
Code.consume_front("err_");
Code.consume_front("-W");
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
