[clang] 26c82f3 - Revert "[clangd] More precisely enable clang warnings through ClangTidy options"
Nico Weber via cfe-commits
cfe-commits at lists.llvm.org
Fri Apr 29 17:34:17 PDT 2022
Author: Nico Weber
Date: 2022-04-29T20:34:10-04:00
New Revision: 26c82f3d1de11cdada57e499b63a05d24e18b656
URL: https://github.com/llvm/llvm-project/commit/26c82f3d1de11cdada57e499b63a05d24e18b656
DIFF: https://github.com/llvm/llvm-project/commit/26c82f3d1de11cdada57e499b63a05d24e18b656.diff
LOG: Revert "[clangd] More precisely enable clang warnings through ClangTidy options"
This reverts commit 5227be8b6aa0edb2edb0b76e1039a7dd5641c80a.
Broke check-clangd, see comment on https://reviews.llvm.org/D124679
Added:
Modified:
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
clang/include/clang/Basic/DiagnosticIDs.h
clang/lib/Basic/DiagnosticIDs.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index 54d8e391cd94a..235362b5d599c 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -233,69 +233,12 @@ class ReplayPreamble : private PPCallbacks {
std::vector<syntax::Token> MainFileTokens;
};
-// Filter for clang diagnostics groups enabled by CTOptions.Checks.
-//
-// These are check names like clang-diagnostics-unused.
-// Note that unlike -Wunused, clang-diagnostics-unused does not imply
-// subcategories like clang-diagnostics-unused-function.
-//
-// This is used to determine which diagnostics can be enabled by ExtraArgs in
-// the clang-tidy configuration.
-class TidyDiagnosticGroups {
- // Whether all diagnostic groups are enabled by default.
- // True if we've seen clang-diagnostic-*.
- bool Default = false;
- // Set of diag::Group whose enablement != Default.
- // If Default is false, this is foo where we've seen clang-diagnostic-foo.
- llvm::DenseSet<unsigned> Exceptions;
-
-public:
- TidyDiagnosticGroups(llvm::StringRef Checks) {
- constexpr llvm::StringLiteral CDPrefix = "clang-diagnostic-";
-
- llvm::StringRef Check;
- while (!Checks.empty()) {
- std::tie(Check, Checks) = Checks.split(',');
- if (Check.empty())
- continue;
-
- bool Enable = !Check.consume_front("-");
- bool Glob = Check.consume_back("*");
- if (Glob) {
- // Is this clang-diagnostic-*, or *, or so?
- // (We ignore all other types of globs).
- if (CDPrefix.startswith(Check)) {
- Default = Enable;
- Exceptions.clear();
- }
- continue;
- }
-
- // In "*,clang-diagnostic-foo", the latter is a no-op.
- if (Default == Enable)
- continue;
- // The only non-glob entries we care about are clang-diagnostic-foo.
- if (!Check.consume_front(CDPrefix))
- continue;
-
- if (auto Group = DiagnosticIDs::getGroupForWarningOption(Check))
- Exceptions.insert(static_cast<unsigned>(*Group));
- }
- }
-
- bool operator()(diag::Group GroupID) const {
- return Exceptions.contains(static_cast<unsigned>(GroupID)) ? !Default
- : Default;
- }
-};
-
// Find -W<group> and -Wno-<group> options in ExtraArgs and apply them to Diags.
//
// This is used to handle ExtraArgs in clang-tidy configuration.
// We don't use clang's standard handling of this as we want slightly
diff erent
// behavior (e.g. we want to exclude these from -Wno-error).
void applyWarningOptions(llvm::ArrayRef<std::string> ExtraArgs,
- llvm::function_ref<bool(diag::Group)> EnabledGroups,
DiagnosticsEngine &Diags) {
for (llvm::StringRef Group : ExtraArgs) {
// Only handle args that are of the form -W[no-]<group>.
@@ -316,9 +259,6 @@ void applyWarningOptions(llvm::ArrayRef<std::string> ExtraArgs,
if (Enable) {
if (Diags.getDiagnosticLevel(ID, SourceLocation()) <
DiagnosticsEngine::Warning) {
- auto Group = DiagnosticIDs::getGroupForDiag(ID);
- if (!Group || !EnabledGroups(*Group))
- continue;
Diags.setSeverity(ID, diag::Severity::Warning, SourceLocation());
if (Diags.getWarningsAsErrors())
NeedsWerrorExclusion = true;
@@ -414,25 +354,18 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
// - ExtraArgs: ["-Wfoo"] causes clang to produce the warnings
// - Checks: "clang-diagnostic-foo" prevents clang-tidy filtering them out
//
- // In clang-tidy, diagnostics are emitted if they pass both checks.
- // When groups contain subgroups, -Wparent includes the child, but
- // clang-diagnostic-parent does not.
+ // We treat these as clang warnings, so the Checks part is not relevant.
+ // We must enable the warnings specified in ExtraArgs.
//
- // We *don't* want to change the compile command directly. This can have
+ // We *don't* want to change the compile command directly. this can have
// too many unexpected effects: breaking the command, interactions with
// -- and -Werror, etc. Besides, we've already parsed the command.
// Instead we parse the -W<group> flags and handle them directly.
- //
- // Similarly, we don't want to use Checks to filter clang diagnostics after
- // they are generated, as this spreads clang-tidy emulation everywhere.
- // Instead, we just use these to filter which extra diagnostics we enable.
auto &Diags = Clang->getDiagnostics();
- TidyDiagnosticGroups TidyGroups(ClangTidyOpts.Checks ? *ClangTidyOpts.Checks
- : llvm::StringRef());
if (ClangTidyOpts.ExtraArgsBefore)
- applyWarningOptions(*ClangTidyOpts.ExtraArgsBefore, TidyGroups, Diags);
+ applyWarningOptions(*ClangTidyOpts.ExtraArgsBefore, Diags);
if (ClangTidyOpts.ExtraArgs)
- applyWarningOptions(*ClangTidyOpts.ExtraArgs, TidyGroups, Diags);
+ applyWarningOptions(*ClangTidyOpts.ExtraArgs, Diags);
} else {
// Skips some analysis.
Clang->getDiagnosticOpts().IgnoreWarnings = true;
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index ca747f686c214..18c16190aeba9 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -517,16 +517,13 @@ TEST(DiagnosticTest, ClangTidyWarningAsError) {
diagSeverity(DiagnosticsEngine::Error)))));
}
-TidyProvider addClangArgs(std::vector<llvm::StringRef> ExtraArgs,
- llvm::StringRef Checks) {
- return [ExtraArgs = std::move(ExtraArgs), Checks = Checks.str()](
- tidy::ClangTidyOptions &Opts, llvm::StringRef) {
+TidyProvider addClangArgs(std::vector<llvm::StringRef> ExtraArgs) {
+ return [ExtraArgs = std::move(ExtraArgs)](tidy::ClangTidyOptions &Opts,
+ llvm::StringRef) {
if (!Opts.ExtraArgs)
Opts.ExtraArgs.emplace();
for (llvm::StringRef Arg : ExtraArgs)
Opts.ExtraArgs->emplace_back(Arg);
- if (!Checks.empty())
- Opts.Checks = Checks;
};
}
@@ -544,78 +541,53 @@ TEST(DiagnosticTest, ClangTidyEnablesClangWarning) {
// Check the -Wunused warning isn't initially on.
EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
- // We enable warnings based on clang-tidy extra args, if the matching
- // clang-diagnostic- is there.
- TU.ClangTidyProvider =
- addClangArgs({"-Wunused"}, "clang-diagnostic-unused-function");
- EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning));
-
- // clang-diagnostic-* is acceptable
- TU.ClangTidyProvider = addClangArgs({"-Wunused"}, "clang-diagnostic-*");
+ // We enable warnings based on clang-tidy extra args.
+ TU.ClangTidyProvider = addClangArgs({"-Wunused"});
EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning));
- // And plain *
- TU.ClangTidyProvider = addClangArgs({"-Wunused"}, "*");
- EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning));
- // And we can explicitly exclude a category too.
- TU.ClangTidyProvider =
- addClangArgs({"-Wunused"}, "*,-clang-diagnostic-unused-function");
- EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
- // Without the exact check specified, the warnings are not enabled.
- TU.ClangTidyProvider = addClangArgs({"-Wunused"}, "clang-diagnostic-unused");
- EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
-
- // We don't respect other args.
- TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Dfoo=bar"},
- "clang-diagnostic-unused-function");
+ // But we don't respect other args.
+ TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Dfoo=bar"});
EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning))
<< "Not unused function 'bar'!";
// -Werror doesn't apply to warnings enabled by clang-tidy extra args.
TU.ExtraArgs = {"-Werror"};
- TU.ClangTidyProvider =
- addClangArgs({"-Wunused"}, "clang-diagnostic-unused-function");
+ TU.ClangTidyProvider = addClangArgs({"-Wunused"});
EXPECT_THAT(*TU.build().getDiagnostics(),
ElementsAre(diagSeverity(DiagnosticsEngine::Warning)));
// But clang-tidy extra args won't *downgrade* errors to warnings either.
TU.ExtraArgs = {"-Wunused", "-Werror"};
- TU.ClangTidyProvider =
- addClangArgs({"-Wunused"}, "clang-diagnostic-unused-function");
+ TU.ClangTidyProvider = addClangArgs({"-Wunused"});
EXPECT_THAT(*TU.build().getDiagnostics(),
ElementsAre(diagSeverity(DiagnosticsEngine::Error)));
// FIXME: we're erroneously downgrading the whole group, this should be Error.
TU.ExtraArgs = {"-Wunused-function", "-Werror"};
- TU.ClangTidyProvider =
- addClangArgs({"-Wunused"}, "clang-diagnostic-unused-label");
+ TU.ClangTidyProvider = addClangArgs({"-Wunused"});
EXPECT_THAT(*TU.build().getDiagnostics(),
ElementsAre(diagSeverity(DiagnosticsEngine::Warning)));
// This looks silly, but it's the typical result if a warning is enabled by a
// high-level .clang-tidy file and disabled by a low-level one.
TU.ExtraArgs = {};
- TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused"},
- "clang-diagnostic-unused-function");
+ TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused"});
EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
// Overriding only works in the proper order.
- TU.ClangTidyProvider =
- addClangArgs({"-Wunused"}, {"clang-diagnostic-unused-function"});
+ TU.ClangTidyProvider = addClangArgs({"-Wno-unused", "-Wunused"});
EXPECT_THAT(*TU.build().getDiagnostics(), SizeIs(1));
// More specific vs less-specific: match clang behavior
- TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused-function"},
- {"clang-diagnostic-unused-function"});
+ TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused-function"});
EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
- TU.ClangTidyProvider = addClangArgs({"-Wunused-function", "-Wno-unused"},
- {"clang-diagnostic-unused-function"});
+ TU.ClangTidyProvider = addClangArgs({"-Wunused-function", "-Wno-unused"});
EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
- // We do allow clang-tidy config to disable warnings from the compile
- // command. It's unclear this is ideal, but it's hard to avoid.
+ // We do allow clang-tidy config to disable warnings from the compile command.
+ // It's unclear this is ideal, but it's hard to avoid.
TU.ExtraArgs = {"-Wunused"};
- TU.ClangTidyProvider = addClangArgs({"-Wno-unused"}, {});
+ TU.ClangTidyProvider = addClangArgs({"-Wno-unused"});
EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
}
diff --git a/clang/include/clang/Basic/DiagnosticIDs.h b/clang/include/clang/Basic/DiagnosticIDs.h
index f27bf356888c4..8139ffd375a28 100644
--- a/clang/include/clang/Basic/DiagnosticIDs.h
+++ b/clang/include/clang/Basic/DiagnosticIDs.h
@@ -231,14 +231,6 @@ class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> {
/// "deprecated-declarations".
static StringRef getWarningOptionForGroup(diag::Group);
- /// Given a group ID, returns the flag that toggles the group.
- /// For example, for "deprecated-declarations", returns
- /// Group::DeprecatedDeclarations.
- static llvm::Optional<diag::Group> getGroupForWarningOption(StringRef);
-
- /// Return the lowest-level group that contains the specified diagnostic.
- static llvm::Optional<diag::Group> getGroupForDiag(unsigned DiagID);
-
/// Return the lowest-level warning option that enables the specified
/// diagnostic.
///
diff --git a/clang/lib/Basic/DiagnosticIDs.cpp b/clang/lib/Basic/DiagnosticIDs.cpp
index a8a17994f7fc8..87db131992e47 100644
--- a/clang/lib/Basic/DiagnosticIDs.cpp
+++ b/clang/lib/Basic/DiagnosticIDs.cpp
@@ -628,27 +628,13 @@ StringRef DiagnosticIDs::getWarningOptionForGroup(diag::Group Group) {
return OptionTable[static_cast<int>(Group)].getName();
}
-llvm::Optional<diag::Group>
-DiagnosticIDs::getGroupForWarningOption(StringRef Name) {
- const auto *Found = llvm::partition_point(
- OptionTable, [=](const WarningOption &O) { return O.getName() < Name; });
- if (Found == std::end(OptionTable) || Found->getName() != Name)
- return llvm::None;
- return static_cast<diag::Group>(Found - OptionTable);
-}
-
-llvm::Optional<diag::Group> DiagnosticIDs::getGroupForDiag(unsigned DiagID) {
- if (const StaticDiagInfoRec *Info = GetDiagInfo(DiagID))
- return static_cast<diag::Group>(Info->getOptionGroupIndex());
- return llvm::None;
-}
-
/// getWarningOptionForDiag - Return the lowest-level warning option that
/// enables the specified diagnostic. If there is no -Wfoo flag that controls
/// the diagnostic, this returns null.
StringRef DiagnosticIDs::getWarningOptionForDiag(unsigned DiagID) {
- if (auto G = getGroupForDiag(DiagID))
- return getWarningOptionForGroup(*G);
+ if (const StaticDiagInfoRec *Info = GetDiagInfo(DiagID))
+ return getWarningOptionForGroup(
+ static_cast<diag::Group>(Info->getOptionGroupIndex()));
return StringRef();
}
@@ -697,10 +683,12 @@ static bool getDiagnosticsInGroup(diag::Flavor Flavor,
bool
DiagnosticIDs::getDiagnosticsInGroup(diag::Flavor Flavor, StringRef Group,
SmallVectorImpl<diag::kind> &Diags) const {
- if (llvm::Optional<diag::Group> G = getGroupForWarningOption(Group))
- return ::getDiagnosticsInGroup(
- Flavor, &OptionTable[static_cast<unsigned>(*G)], Diags);
- return true;
+ auto Found = llvm::partition_point(
+ OptionTable, [=](const WarningOption &O) { return O.getName() < Group; });
+ if (Found == std::end(OptionTable) || Found->getName() != Group)
+ return true; // Option not found.
+
+ return ::getDiagnosticsInGroup(Flavor, Found, Diags);
}
void DiagnosticIDs::getAllDiagnostics(diag::Flavor Flavor,
More information about the cfe-commits
mailing list