llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Kryptonite (OmarAzizi) <details> <summary>Changes</summary> Adds a new clang-tidy check that detects structured bindings where a binding is only used to suppress an unused-variable warning, and replaces it with C++26's placeholder `_`, removing the suppression statement. Excludes `static`/`thread_local` decompositions, since `_` is only exempt from same-scope redefinition rules for automatic-storage-duration declarations. Fixes #<!-- -->190387 Assisted by Claude Code, all decisions taken in this PR involved my active judgment --- Full diff: https://github.com/llvm/llvm-project/pull/207604.diff 8 Files Affected: - (modified) clang-tools-extra/clang-tidy/modernize/CMakeLists.txt (+1) - (modified) clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp (+3) - (added) clang-tools-extra/clang-tidy/modernize/UsePlaceholderBindingCheck.cpp (+88) - (added) clang-tools-extra/clang-tidy/modernize/UsePlaceholderBindingCheck.h (+35) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+7) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1) - (added) clang-tools-extra/docs/clang-tidy/checks/modernize/use-placeholder-binding.rst (+47) - (added) clang-tools-extra/test/clang-tidy/checkers/modernize/use-placeholder-binding.cpp (+163) ``````````diff diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt index 2c5c44db587fe..dca3abd63756f 100644 --- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt @@ -44,6 +44,7 @@ add_clang_library(clangTidyModernizeModule STATIC UseNoexceptCheck.cpp UseNullptrCheck.cpp UseOverrideCheck.cpp + UsePlaceholderBindingCheck.cpp UseRangesCheck.cpp UseScopedLockCheck.cpp UseStartsEndsWithCheck.cpp diff --git a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp index cc13da7535bcb..32241ea319b78 100644 --- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp @@ -44,6 +44,7 @@ #include "UseNoexceptCheck.h" #include "UseNullptrCheck.h" #include "UseOverrideCheck.h" +#include "UsePlaceholderBindingCheck.h" #include "UseRangesCheck.h" #include "UseScopedLockCheck.h" #include "UseStartsEndsWithCheck.h" @@ -92,6 +93,8 @@ class ModernizeModule : public ClangTidyModule { "modernize-use-designated-initializers"); CheckFactories.registerCheck<UseIntegerSignComparisonCheck>( "modernize-use-integer-sign-comparison"); + CheckFactories.registerCheck<UsePlaceholderBindingCheck>( + "modernize-use-placeholder-binding"); CheckFactories.registerCheck<UseRangesCheck>("modernize-use-ranges"); CheckFactories.registerCheck<UseScopedLockCheck>( "modernize-use-scoped-lock"); diff --git a/clang-tools-extra/clang-tidy/modernize/UsePlaceholderBindingCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UsePlaceholderBindingCheck.cpp new file mode 100644 index 0000000000000..8411b4396ff53 --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/UsePlaceholderBindingCheck.cpp @@ -0,0 +1,88 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "UsePlaceholderBindingCheck.h" +#include "../utils/DeclRefExprUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::modernize { + +namespace { +AST_POLYMORPHIC_MATCHER(isInMacro, + AST_POLYMORPHIC_SUPPORTED_TYPES(Stmt, Decl)) { + return Node.getBeginLoc().isMacroID() || Node.getEndLoc().isMacroID(); +} + +AST_MATCHER(VarDecl, hasAutomaticStorageDurationAndNoSpecifiers) { + return !Node.isStaticLocal() && !Node.isConstexpr() && !Node.hasAttrs() && + Node.getStorageClass() == SC_None && + Node.getTSCSpec() == TSCS_unspecified; +} +} // namespace + +void UsePlaceholderBindingCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + decompositionDecl(unless(isInMacro()), + hasAutomaticStorageDurationAndNoSpecifiers(), + hasAncestor(compoundStmt().bind("scope"))) + .bind("decomp"), + this); +} + +void UsePlaceholderBindingCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Decomp = Result.Nodes.getNodeAs<DecompositionDecl>("decomp"); + const auto *Scope = Result.Nodes.getNodeAs<CompoundStmt>("scope"); + ASTContext &Context = *Result.Context; + + for (const BindingDecl *Binding : Decomp->bindings()) { + if (Binding->isParameterPack() || + Binding->isPlaceholderVar(Context.getLangOpts())) + continue; + + llvm::SmallPtrSet<const DeclRefExpr *, 16> Refs = + utils::decl_ref_expr::allDeclRefExprs(*Binding, *Scope, Context); + if (Refs.size() != 1) + continue; + + const DeclRefExpr *Ref = *Refs.begin(); + + const DynTypedNodeList RefParents = Context.getParents(*Ref); + if (RefParents.size() != 1) + continue; + const auto *Cast = + dyn_cast_or_null<CStyleCastExpr>(RefParents[0].get<Expr>()); + if (!Cast || !Cast->getType()->isVoidType() || + Cast->getSubExpr()->IgnoreParens() != Ref || + Cast->getBeginLoc().isMacroID() || Cast->getEndLoc().isMacroID()) + continue; + + const DynTypedNodeList CastParents = Context.getParents(*Cast); + if (CastParents.size() != 1 || !CastParents[0].get<CompoundStmt>()) + continue; + + const SourceLocation SemiLoc = Lexer::findLocationAfterToken( + Cast->getEndLoc(), tok::semi, Context.getSourceManager(), + Context.getLangOpts(), /*SkipTrailingWhitespaceAndNewLine=*/true); + if (SemiLoc.isInvalid()) + continue; + + diag(Binding->getLocation(), + "binding %0 is only used to suppress an unused variable warning; " + "use a placeholder '_' instead") + << Binding + << FixItHint::CreateReplacement(Binding->getSourceRange(), "_") + << FixItHint::CreateRemoval( + CharSourceRange::getCharRange(Cast->getBeginLoc(), SemiLoc)); + } +} + +} // namespace clang::tidy::modernize diff --git a/clang-tools-extra/clang-tidy/modernize/UsePlaceholderBindingCheck.h b/clang-tools-extra/clang-tidy/modernize/UsePlaceholderBindingCheck.h new file mode 100644 index 0000000000000..4fff939f99138 --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/UsePlaceholderBindingCheck.h @@ -0,0 +1,35 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEPLACEHOLDERBINDINGCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEPLACEHOLDERBINDINGCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::modernize { + +/// Finds structured bindings where a binding is only used to suppress an +/// "unused variable" warning via a ``(void)name;`` statement, and suggests +/// replacing the binding with a C++26 placeholder (``_``) instead. +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-placeholder-binding.html +class UsePlaceholderBindingCheck : public ClangTidyCheck { +public: + UsePlaceholderBindingCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus26; + } +}; + +} // namespace clang::tidy::modernize + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEPLACEHOLDERBINDINGCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 68f0fa568dbf9..018d0cdda8fba 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -265,6 +265,13 @@ New checks Finds cyclical initialization of static variables. +- New :doc:`modernize-use-placeholder-binding + <clang-tidy/checks/modernize/use-placeholder-binding>` check. + + Finds structured bindings where a binding is only used to suppress an + unused variable warning and suggests replacing it with a C++26 + placeholder (``_``). + - New :doc:`modernize-use-std-bit <clang-tidy/checks/modernize/use-std-bit>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 2a44dc78fbc89..4e1f98f29c08a 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -325,6 +325,7 @@ Clang-Tidy Checks :doc:`modernize-use-noexcept <modernize/use-noexcept>`, "Yes" :doc:`modernize-use-nullptr <modernize/use-nullptr>`, "Yes" :doc:`modernize-use-override <modernize/use-override>`, "Yes" + :doc:`modernize-use-placeholder-binding <modernize/use-placeholder-binding>`, "Yes" :doc:`modernize-use-ranges <modernize/use-ranges>`, "Yes" :doc:`modernize-use-scoped-lock <modernize/use-scoped-lock>`, "Yes" :doc:`modernize-use-starts-ends-with <modernize/use-starts-ends-with>`, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-placeholder-binding.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-placeholder-binding.rst new file mode 100644 index 0000000000000..0f847454e30ae --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-placeholder-binding.rst @@ -0,0 +1,47 @@ +.. title:: clang-tidy - modernize-use-placeholder-binding + +modernize-use-placeholder-binding +================================== + +Finds structured bindings where one of the bindings is only used to suppress +an "unused variable" warning via a ``(void)name;`` statement, and suggests +replacing that binding with a C++26 placeholder (``_``) instead, removing the +now unnecessary suppression statement. + +.. code-block:: c++ + + const auto [x, y] = compute(); + (void)y; + use(x); + + // becomes + + const auto [x, _] = compute(); + use(x); + +This also applies to structured bindings declared in the init-statement of an +``if``, ``switch``, or ``for`` statement: + +.. code-block:: c++ + + if (const auto [it, inserted] = set.insert(value); inserted) { + (void)it; + // ... + } + + // becomes + + if (const auto [_, inserted] = set.insert(value); inserted) { + // ... + } + +Limitations +----------- + +The check only recognizes the ``(void)name;`` suppression idiom as a +standalone statement directly inside a compound statement. It does not +diagnose bindings that are simply unused with no suppression statement, nor +bindings suppressed some other way. It also does not diagnose a suppression +cast that is the sole substatement of a ``case`` or ``default`` label (for +example, inside a ``switch``), since removing it safely would require +special-casing the label rather than the statement. diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-placeholder-binding.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-placeholder-binding.cpp new file mode 100644 index 0000000000000..943b873c63023 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-placeholder-binding.cpp @@ -0,0 +1,163 @@ +// RUN: %check_clang_tidy -std=c++26-or-later %s modernize-use-placeholder-binding %t + +struct Pair { + int first; + bool second; +}; + +struct Triple { + int a, b, c; +}; + +Pair getPair(); +Triple getTriple(); +void use(int); + +void suppressedBindingIsReplaced() { + const auto [x, y] = getPair(); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: binding 'y' is only used to suppress an unused variable warning; use a placeholder '_' instead [modernize-use-placeholder-binding] + // CHECK-FIXES: const auto [x, _] = getPair(); + (void)y; + // CHECK-FIXES-NOT: (void)y; + use(x); +} + +void ifInitBindingIsReplaced() { + if (const auto [it, inserted] = getPair(); inserted) { + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: binding 'it' is only used to suppress an unused variable warning; use a placeholder '_' instead [modernize-use-placeholder-binding] + // CHECK-FIXES: if (const auto [_, inserted] = getPair(); inserted) { + (void)it; + // CHECK-FIXES-NOT: (void)it; + } +} + +void bothBindingsUsed() { + const auto [x, y] = getPair(); + use(x); + if (y) + use(x); +} + +void noSuppressionCast() { + const auto [x, y] = getPair(); + use(x); +} + +void alreadyPlaceholder() { + const auto [x, _] = getPair(); + use(x); +} + +void bothBindingsSuppressed() { + auto [x, y] = getPair(); + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: binding 'x' is only used to suppress an unused variable warning; use a placeholder '_' instead [modernize-use-placeholder-binding] + // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: binding 'y' is only used to suppress an unused variable warning; use a placeholder '_' instead [modernize-use-placeholder-binding] + // CHECK-FIXES: auto [_, _] = getPair(); + (void)x; + // CHECK-FIXES-NOT: (void)x; + (void)y; + // CHECK-FIXES-NOT: (void)y; +} + +void threeBindings() { + auto [a, b, c] = getTriple(); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: binding 'b' is only used to suppress an unused variable warning; use a placeholder '_' instead [modernize-use-placeholder-binding] + // CHECK-MESSAGES: :[[@LINE-2]]:15: warning: binding 'c' is only used to suppress an unused variable warning; use a placeholder '_' instead [modernize-use-placeholder-binding] + // CHECK-FIXES: auto [a, _, _] = getTriple(); + (void)b; + (void)c; + use(a); +} + +void referenceBinding() { + Pair p{}; + auto &[x, y] = p; + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: binding 'y' is only used to suppress an unused variable warning; use a placeholder '_' instead [modernize-use-placeholder-binding] + // CHECK-FIXES: auto &[x, _] = p; + (void)y; + use(x); +} + +struct Item { + int key; + int value; +}; + +void forRangeBindingIsReplaced() { + Item items[1] = {}; + for (auto [k, v] : items) { + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: binding 'v' is only used to suppress an unused variable warning; use a placeholder '_' instead [modernize-use-placeholder-binding] + // CHECK-FIXES: for (auto [k, _] : items) { + (void)v; + // CHECK-FIXES-NOT: (void)v; + use(k); + } +} + +void switchCaseSuppressionIsNotDiagnosed() { + switch (auto [a, b] = getPair(); a) { + case 0: + (void)b; + break; + } +} + +void lambdaCaptureIsUnaffected() { + auto [x, y] = getPair(); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: binding 'y' is only used to suppress an unused variable warning; use a placeholder '_' instead [modernize-use-placeholder-binding] + // CHECK-FIXES: auto [x, _] = getPair(); + (void)y; + // CHECK-FIXES-NOT: (void)y; + auto l = [x] { use(x); }; + l(); +} + +#define SUPPRESS(x) (void)(x) +void macroSuppressionIsNotDiagnosed() { + auto [x, y] = getPair(); + SUPPRESS(y); + use(x); +} + +void multipleDecompositionsInSameScope() { + auto [x1, y1] = getPair(); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: binding 'y1' is only used to suppress an unused variable warning; use a placeholder '_' instead [modernize-use-placeholder-binding] + // CHECK-FIXES: auto [x1, _] = getPair(); + (void)y1; + // CHECK-FIXES-NOT: (void)y1; + use(x1); + + auto [x2, y2] = getPair(); + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: binding 'x2' is only used to suppress an unused variable warning; use a placeholder '_' instead [modernize-use-placeholder-binding] + // CHECK-FIXES: auto [_, y2] = getPair(); + (void)x2; + // CHECK-FIXES-NOT: (void)x2; + use(y2 ? 1 : 0); +} + +Pair &&getPairRef(); + +void rvalueReferenceBinding() { + auto &&[x, y] = getPairRef(); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: binding 'y' is only used to suppress an unused variable warning; use a placeholder '_' instead [modernize-use-placeholder-binding] + // CHECK-FIXES: auto &&[x, _] = getPairRef(); + (void)y; + // CHECK-FIXES-NOT: (void)y; + use(x); + // CHECK-FIXES: use(x); +} + +void staticBindingIsNotDiagnosed() { + static auto [x1, y1] = getPair(); + (void)y1; + static auto [x2, y2] = getPair(); + (void)y2; + use(x1); + use(x2); +} + +void threadLocalBindingIsNotDiagnosed() { + thread_local auto [x, y] = getPair(); + (void)y; + use(x); +} `````````` </details> https://github.com/llvm/llvm-project/pull/207604 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
