diff --git a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp index 5bfa6fb0d02d5..655e480ffa62c 100644 --- a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp @@ -97,6 +97,15 @@ bool ForRangeCopyCheck::handleConstValueCopy(const VarDecl &LoopVar, return true; } +static bool isReferenced(const VarDecl &LoopVar, const Stmt &Stmt, + ASTContext &Context) { + const auto IsLoopVar = varDecl(equalsNode(&LoopVar)); + return !match(stmt(hasDescendant(declRefExpr(to(valueDecl(anyOf( + IsLoopVar, bindingDecl(forDecomposition(IsLoopVar)))))))), + Stmt, Context) + .empty(); +} + bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced( const VarDecl &LoopVar, const CXXForRangeStmt &ForRange, ASTContext &Context) { @@ -113,9 +122,7 @@ bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced( // compiler warning which can't be suppressed. // Since this case is very rare, it is safe to ignore it. if (!ExprMutationAnalyzer(*ForRange.getBody(), Context).isMutated(&LoopVar) && - !utils::decl_ref_expr::allDeclRefExprs(LoopVar, *ForRange.getBody(), - Context) - .empty()) { + isReferenced(LoopVar, *ForRange.getBody(), Context)) { auto Diag = diag( LoopVar.getLocation(), "loop variable is copied but only used as const reference; consider " diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index a235a7d02592e..84c1e90698573 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -447,6 +447,10 @@ Changes in existing checks ` check to properly escape single quotes. +- Improved :doc:`performance-for-range-copy + ` check to handle cases where + the loop variable is a structured binding. + - Improved :doc:`performance-noexcept-move-constructor ` to better handle conditional ``noexcept`` expressions, eliminating false-positives. diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp index 1a2eedc9e65c5..f9d06898ca03d 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp @@ -47,6 +47,11 @@ struct S { S &operator=(const S &); }; +struct Point { + ~Point() {} + int x, y; +}; + struct Convertible { operator S() const { return S(); @@ -87,6 +92,10 @@ void instantiated() { // CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is {{.*}} // CHECK-FIXES: {{^}} for (const S& S2 : View>()) {} + for (const auto [X, Y] : View>()) {} + // CHECK-MESSAGES: [[@LINE-1]]:19: warning: the loop variable's type is + // CHECK-FIXES: {{^}} for (const auto& [X, Y] : View>()) {} + for (const T T2 : View>()) {} // CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is {{.*}} // CHECK-FIXES: {{^}} for (const T& T2 : View>()) {} @@ -123,11 +132,6 @@ struct Mutable { ~Mutable() {} }; -struct Point { - ~Point() {} - int x, y; -}; - Mutable& operator<<(Mutable &Out, bool B) { Out.setBool(B); return Out; @@ -144,6 +148,7 @@ void useByValue(Mutable M); void useByConstValue(const Mutable M); void mutate(Mutable *M); void mutate(Mutable &M); +void mutate(int &); void onceConstOnceMutated(const Mutable &M1, Mutable &M2); void negativeVariableIsMutated() { @@ -234,6 +239,22 @@ void positiveOnlyAccessedFieldAsConst() { } } +void positiveOnlyUsedAsConstBinding() { + for (auto [X, Y] : View>()) { + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but + // CHECK-FIXES: for (const auto& [X, Y] : View>()) { + use(X); + use(Y); + } +} + +void negativeMutatedBinding() { + for (auto [X, Y] : View>()) { + use(X); + mutate(Y); + } +} + void positiveOnlyUsedInCopyConstructor() { for (auto A : View>()) { // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy] diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp index c0de9277ff866..9af818be0415f 100644 --- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp +++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp @@ -243,9 +243,16 @@ const Stmt *ExprMutationAnalyzer::findMutationMemoized( const Stmt *ExprMutationAnalyzer::tryEachDeclRef(const Decl *Dec, MutationFinder Finder) { - const auto Refs = - match(findAll(declRefExpr(to(equalsNode(Dec))).bind(NodeID::value)), - Stm, Context); + const auto Refs = match( + findAll( + declRefExpr(to( + // `Dec` or a binding if `Dec` is a decomposition. + anyOf(equalsNode(Dec), + bindingDecl(forDecomposition(equalsNode(Dec)))) + // + )) + .bind(NodeID::value)), + Stm, Context); for (const auto &RefNodes : Refs) { const auto *E = RefNodes.getNodeAs(NodeID::value); if ((this->*Finder)(E)) diff --git a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp index f1a1f857a0ee5..a94f857720b03 100644 --- a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp +++ b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp @@ -592,6 +592,26 @@ TEST(ExprMutationAnalyzerTest, ByConstRRefArgument) { ElementsAre("static_cast(x)")); } +// Section: bindings. + +TEST(ExprMutationAnalyzerTest, BindingModifies) { + const auto AST = + buildASTFromCode("struct Point { int x; int y; };" + "void mod(int&);" + "void f(Point p) { auto& [x, y] = p; mod(x); }"); + const auto Results = + match(withEnclosingCompound(declRefTo("p")), AST->getASTContext()); + EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x", "mod(x)")); +} + +TEST(ExprMutationAnalyzerTest, BindingDoesNotModify) { + const auto AST = buildASTFromCode("struct Point { int x; int y; };" + "void f(Point p) { auto& [x, y] = p; x; }"); + const auto Results = + match(withEnclosingCompound(declRefTo("p")), AST->getASTContext()); + EXPECT_FALSE(isMutated(Results, AST.get())); +} + // section: explicit std::move and std::forward testing TEST(ExprMutationAnalyzerTest, Move) {