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, _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits