https://github.com/segoon updated https://github.com/llvm/llvm-project/pull/139525
>From 3abbce9f817f6d09f9a5b7549a8122c80821eaf8 Mon Sep 17 00:00:00 2001 From: Vasily Kulikov <seg...@yandex-team.ru> Date: Mon, 12 May 2025 13:05:43 +0300 Subject: [PATCH 01/14] [clang-tidy] Add check performance-lost-std-move --- .../clang-tidy/performance/CMakeLists.txt | 1 + .../performance/LostStdMoveCheck.cpp | 96 ++++++++++++++++ .../clang-tidy/performance/LostStdMoveCheck.h | 37 ++++++ .../performance/PerformanceTidyModule.cpp | 2 + clang-tools-extra/docs/ReleaseNotes.rst | 5 + .../docs/clang-tidy/checks/list.rst | 1 + .../checks/performance/lost-std-move.rst | 14 +++ .../checkers/performance/lost-std-move.cpp | 108 ++++++++++++++++++ 8 files changed, 264 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/performance/lost-std-move.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt index 81128ff086021..333abd10a583a 100644 --- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt @@ -12,6 +12,7 @@ add_clang_library(clangTidyPerformanceModule InefficientAlgorithmCheck.cpp InefficientStringConcatenationCheck.cpp InefficientVectorOperationCheck.cpp + LostStdMoveCheck.cpp MoveConstArgCheck.cpp MoveConstructorInitCheck.cpp NoAutomaticMoveCheck.cpp diff --git a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp new file mode 100644 index 0000000000000..26148e1d26de9 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp @@ -0,0 +1,96 @@ +//===--- LostStdMoveCheck.cpp - clang-tidy --------------------------------===// +// +// 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 "LostStdMoveCheck.h" +#include "../utils/DeclRefExprUtils.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::performance { + +using utils::decl_ref_expr::allDeclRefExprs; + +AST_MATCHER(CXXRecordDecl, hasTrivialMoveConstructor) { + return Node.hasDefinition() && Node.hasTrivialMoveConstructor(); +} + +void LostStdMoveCheck::registerMatchers(MatchFinder *Finder) { + auto returnParent = + hasParent(expr(hasParent(cxxConstructExpr(hasParent(returnStmt()))))); + + Finder->addMatcher( + declRefExpr( + // not "return x;" + unless(returnParent), + + unless(hasType(namedDecl(hasName("::std::string_view")))), + + // non-trivial type + hasType(hasCanonicalType(hasDeclaration(cxxRecordDecl()))), + + // non-trivial X(X&&) + unless(hasType(hasCanonicalType( + hasDeclaration(cxxRecordDecl(hasTrivialMoveConstructor()))))), + + // Not in a cycle + unless(hasAncestor(forStmt())), unless(hasAncestor(doStmt())), + unless(hasAncestor(whileStmt())), + + // only non-X& + unless(hasDeclaration( + varDecl(hasType(qualType(lValueReferenceType()))))), + + hasDeclaration( + varDecl(hasAncestor(functionDecl().bind("func"))).bind("decl")), + + hasParent(expr(hasParent(cxxConstructExpr())).bind("use_parent"))) + .bind("use"), + this); +} + +const Expr *LostStdMoveCheck::getLastVarUsage(const VarDecl &Var, + const Decl &Func, + ASTContext &Context) { + auto Exprs = allDeclRefExprs(Var, Func, Context); + + const Expr *LastExpr = nullptr; + for (const auto &Expr : Exprs) { + if (!LastExpr) + LastExpr = Expr; + + if (LastExpr->getBeginLoc() < Expr->getBeginLoc()) + LastExpr = Expr; + } + + return LastExpr; +} + +void LostStdMoveCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MatchedDecl = Result.Nodes.getNodeAs<VarDecl>("decl"); + const auto *MatchedFunc = Result.Nodes.getNodeAs<FunctionDecl>("func"); + const auto *MatchedUse = Result.Nodes.getNodeAs<Expr>("use"); + const auto *MatchedUseCall = Result.Nodes.getNodeAs<CallExpr>("use_parent"); + + if (MatchedUseCall) + return; + + const auto *LastUsage = + getLastVarUsage(*MatchedDecl, *MatchedFunc, *Result.Context); + if (LastUsage == nullptr) + return; + + if (LastUsage->getBeginLoc() > MatchedUse->getBeginLoc()) { + // "use" is not the last reference to x + return; + } + + diag(LastUsage->getBeginLoc(), "Could be std::move()"); +} + +} // namespace clang::tidy::performance diff --git a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.h b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.h new file mode 100644 index 0000000000000..c62c3f6448a82 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.h @@ -0,0 +1,37 @@ +//===--- LostStdMoveCheck.h - clang-tidy ------------------------*- C++ -*-===// +// +// 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_PERFORMANCE_LOSTSTDMOVECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_LOSTSTDMOVECHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::performance { + +/// Search for lost std::move(). +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/performance/lost-std-move.html +class LostStdMoveCheck : public ClangTidyCheck { +public: + LostStdMoveCheck(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.CPlusPlus; + } + +private: + const Expr *getLastVarUsage(const VarDecl &Var, const Decl &Func, + ASTContext &Context); +}; + +} // namespace clang::tidy::performance + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_LOSTSTDMOVECHECK_H diff --git a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp index 9e0fa6f88b36a..6c45f8678fe63 100644 --- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp @@ -17,6 +17,7 @@ #include "InefficientAlgorithmCheck.h" #include "InefficientStringConcatenationCheck.h" #include "InefficientVectorOperationCheck.h" +#include "LostStdMoveCheck.h" #include "MoveConstArgCheck.h" #include "MoveConstructorInitCheck.h" #include "NoAutomaticMoveCheck.h" @@ -49,6 +50,7 @@ class PerformanceModule : public ClangTidyModule { "performance-inefficient-string-concatenation"); CheckFactories.registerCheck<InefficientVectorOperationCheck>( "performance-inefficient-vector-operation"); + CheckFactories.registerCheck<LostStdMoveCheck>("performance-lost-std-move"); CheckFactories.registerCheck<MoveConstArgCheck>( "performance-move-const-arg"); CheckFactories.registerCheck<MoveConstructorInitCheck>( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 6d91748e4cef1..17da163ff041c 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -212,6 +212,11 @@ New checks Recommends the smallest possible underlying type for an ``enum`` or ``enum`` class based on the range of its enumerators. +- New :doc:`performance-lost-std-move + <clang-tidy/checks/performance/lost-std-move>` check. + + Searches for lost std::move(). + - New :doc:`readability-reference-to-constructed-temporary <clang-tidy/checks/readability/reference-to-constructed-temporary>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 31f0e090db1d7..2eba4aacb2c33 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -321,6 +321,7 @@ Clang-Tidy Checks :doc:`performance-inefficient-algorithm <performance/inefficient-algorithm>`, "Yes" :doc:`performance-inefficient-string-concatenation <performance/inefficient-string-concatenation>`, :doc:`performance-inefficient-vector-operation <performance/inefficient-vector-operation>`, "Yes" + :doc:`performance-lost-std-move <performance/lost-std-move>`, "Yes" :doc:`performance-move-const-arg <performance/move-const-arg>`, "Yes" :doc:`performance-move-constructor-init <performance/move-constructor-init>`, :doc:`performance-no-automatic-move <performance/no-automatic-move>`, diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/lost-std-move.rst b/clang-tools-extra/docs/clang-tidy/checks/performance/lost-std-move.rst new file mode 100644 index 0000000000000..ded49de7b8126 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/performance/lost-std-move.rst @@ -0,0 +1,14 @@ +.. title:: clang-tidy - performance-lost-std-move + +performance-lost-std-move +========================= + +The check warns if copy constructor is used instead of std::move(). + +.. code-block:: c++ + + void f(X); + + void g(X x) { + f(x); // warning: Could be std::move() [performance-lost-std-move] + } diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp new file mode 100644 index 0000000000000..ce2d1b972dbd5 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp @@ -0,0 +1,108 @@ +// RUN: %check_clang_tidy %s performance-lost-std-move %t + +namespace std { + +template<typename T> +class shared_ptr { +public: + T& operator*() { return reinterpret_cast<T&>(*this); } + shared_ptr() {} + shared_ptr(const shared_ptr<T>&) {} +}; + +template<typename T> +T&& move(T&) +{ +} + +} // namespace std + +int f(std::shared_ptr<int>); + +void f_arg(std::shared_ptr<int> ptr) +{ + if (*ptr) + f(ptr); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Could be std::move() [performance-lost-std-move] +} + +void f_rvalue_ref(std::shared_ptr<int>&& ptr) +{ + if (*ptr) + f(ptr); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Could be std::move() [performance-lost-std-move] +} + +using SharedPtr = std::shared_ptr<int>; +void f_using(SharedPtr ptr) +{ + if (*ptr) + f(ptr); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Could be std::move() [performance-lost-std-move] +} + +void f_local() +{ + std::shared_ptr<int> ptr; + if (*ptr) + f(ptr); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Could be std::move() [performance-lost-std-move] +} + +void f_move() +{ + std::shared_ptr<int> ptr; + if (*ptr) + f(std::move(ptr)); +} + +void f_ref(std::shared_ptr<int> &ptr) +{ + if (*ptr) + f(ptr); +} + +std::shared_ptr<int> f_return() +{ + std::shared_ptr<int> ptr; + return ptr; +} + +void f_still_used(std::shared_ptr<int> ptr) +{ + if (*ptr) + f(ptr); + + *ptr = 1; + *ptr = *ptr; +} + +void f_cycle1() +{ + std::shared_ptr<int> ptr; + for(;;) + f(ptr); +} + +void f_cycle2() +{ + std::shared_ptr<int> ptr; + for(int i=0; i<5; i++) + f(ptr); +} + +void f_cycle3() +{ + std::shared_ptr<int> ptr; + while (*ptr) { + f(ptr); + } +} + +void f_cycle4() +{ + std::shared_ptr<int> ptr; + do { + f(ptr); + } while (*ptr); +} >From 4a1a77baacaec68c577e8fd6bb4cfe58d7496ac0 Mon Sep 17 00:00:00 2001 From: Vasily Kulikov <seg...@yandex-team.ru> Date: Mon, 12 May 2025 15:13:14 +0300 Subject: [PATCH 02/14] f(x, x) --- .../performance/LostStdMoveCheck.cpp | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp index 26148e1d26de9..228d654b39aef 100644 --- a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp @@ -24,6 +24,9 @@ void LostStdMoveCheck::registerMatchers(MatchFinder *Finder) { auto returnParent = hasParent(expr(hasParent(cxxConstructExpr(hasParent(returnStmt()))))); + auto outermostExpr = expr(unless(hasParent(expr()))); + auto leafStatement = stmt(outermostExpr, unless(hasDescendant(outermostExpr))); + Finder->addMatcher( declRefExpr( // not "return x;" @@ -46,6 +49,8 @@ void LostStdMoveCheck::registerMatchers(MatchFinder *Finder) { unless(hasDeclaration( varDecl(hasType(qualType(lValueReferenceType()))))), + hasAncestor(leafStatement.bind("leaf_statement")), + hasDeclaration( varDecl(hasAncestor(functionDecl().bind("func"))).bind("decl")), @@ -71,11 +76,19 @@ const Expr *LostStdMoveCheck::getLastVarUsage(const VarDecl &Var, return LastExpr; } +template <typename Node> +void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID, + llvm::SmallPtrSet<const Node *, 16> &Nodes) { + for (const auto &Match : Matches) + Nodes.insert(Match.getNodeAs<Node>(ID)); +} + void LostStdMoveCheck::check(const MatchFinder::MatchResult &Result) { const auto *MatchedDecl = Result.Nodes.getNodeAs<VarDecl>("decl"); const auto *MatchedFunc = Result.Nodes.getNodeAs<FunctionDecl>("func"); const auto *MatchedUse = Result.Nodes.getNodeAs<Expr>("use"); const auto *MatchedUseCall = Result.Nodes.getNodeAs<CallExpr>("use_parent"); + const auto *MatchedLeafStatement = Result.Nodes.getNodeAs<Stmt>("leaf_statement"); if (MatchedUseCall) return; @@ -90,6 +103,15 @@ void LostStdMoveCheck::check(const MatchFinder::MatchResult &Result) { return; } + // Calculate X usage count in the statement + llvm::SmallPtrSet<const DeclRefExpr *, 16> DeclRefs; + auto Matches = match(findAll(declRefExpr(to(varDecl(equalsNode(MatchedDecl)))).bind("ref")), *MatchedLeafStatement, *Result.Context); + extractNodesByIdTo(Matches, "ref", DeclRefs); + if (DeclRefs.size() > 1) { + // Unspecified order of evaluation, e.g. f(x, x) + return; + } + diag(LastUsage->getBeginLoc(), "Could be std::move()"); } >From f74066dffc49aef6c77417021c134c001820fa8b Mon Sep 17 00:00:00 2001 From: Vasily Kulikov <seg...@yandex-team.ru> Date: Mon, 12 May 2025 15:14:18 +0300 Subject: [PATCH 03/14] test --- .../test/clang-tidy/checkers/performance/lost-std-move.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp index ce2d1b972dbd5..a4a2550971cac 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp @@ -106,3 +106,9 @@ void f_cycle4() f(ptr); } while (*ptr); } + +int f_multiple_usages() +{ + std::shared_ptr<int> ptr; + return f(ptr) + f(ptr); +} >From edd4800851de89dc3e70b5b483e89fb3c0220fbc Mon Sep 17 00:00:00 2001 From: Vasily Kulikov <seg...@yandex-team.ru> Date: Mon, 26 May 2025 12:37:03 +0300 Subject: [PATCH 04/14] review fixes --- .../performance/LostStdMoveCheck.cpp | 73 +++++++++---------- .../clang-tidy/performance/LostStdMoveCheck.h | 6 +- 2 files changed, 37 insertions(+), 42 deletions(-) diff --git a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp index 228d654b39aef..cb6700fca8489 100644 --- a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp @@ -16,16 +16,31 @@ namespace clang::tidy::performance { using utils::decl_ref_expr::allDeclRefExprs; +static const Expr* getLastVarUsage(const VarDecl& Var, const Decl& Func, + ASTContext& Context) { + auto Exprs = allDeclRefExprs(Var, Func, Context); + + const Expr* LastExpr = nullptr; + for (const clang::DeclRefExpr* Expr : Exprs) { + if (!LastExpr) LastExpr = Expr; + + if (LastExpr->getBeginLoc() < Expr->getBeginLoc()) LastExpr = Expr; + } + + return LastExpr; +} + AST_MATCHER(CXXRecordDecl, hasTrivialMoveConstructor) { return Node.hasDefinition() && Node.hasTrivialMoveConstructor(); } -void LostStdMoveCheck::registerMatchers(MatchFinder *Finder) { +void LostStdMoveCheck::registerMatchers(MatchFinder* Finder) { auto returnParent = hasParent(expr(hasParent(cxxConstructExpr(hasParent(returnStmt()))))); auto outermostExpr = expr(unless(hasParent(expr()))); - auto leafStatement = stmt(outermostExpr, unless(hasDescendant(outermostExpr))); + auto leafStatement = + stmt(outermostExpr, unless(hasDescendant(outermostExpr))); Finder->addMatcher( declRefExpr( @@ -49,7 +64,7 @@ void LostStdMoveCheck::registerMatchers(MatchFinder *Finder) { unless(hasDeclaration( varDecl(hasType(qualType(lValueReferenceType()))))), - hasAncestor(leafStatement.bind("leaf_statement")), + hasAncestor(leafStatement.bind("leaf_statement")), hasDeclaration( varDecl(hasAncestor(functionDecl().bind("func"))).bind("decl")), @@ -59,44 +74,26 @@ void LostStdMoveCheck::registerMatchers(MatchFinder *Finder) { this); } -const Expr *LostStdMoveCheck::getLastVarUsage(const VarDecl &Var, - const Decl &Func, - ASTContext &Context) { - auto Exprs = allDeclRefExprs(Var, Func, Context); - - const Expr *LastExpr = nullptr; - for (const auto &Expr : Exprs) { - if (!LastExpr) - LastExpr = Expr; - - if (LastExpr->getBeginLoc() < Expr->getBeginLoc()) - LastExpr = Expr; - } - - return LastExpr; -} - template <typename Node> void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID, - llvm::SmallPtrSet<const Node *, 16> &Nodes) { - for (const auto &Match : Matches) + llvm::SmallPtrSet<const Node*, 16>& Nodes) { + for (const BoundNodes& Match : Matches) Nodes.insert(Match.getNodeAs<Node>(ID)); } -void LostStdMoveCheck::check(const MatchFinder::MatchResult &Result) { - const auto *MatchedDecl = Result.Nodes.getNodeAs<VarDecl>("decl"); - const auto *MatchedFunc = Result.Nodes.getNodeAs<FunctionDecl>("func"); - const auto *MatchedUse = Result.Nodes.getNodeAs<Expr>("use"); - const auto *MatchedUseCall = Result.Nodes.getNodeAs<CallExpr>("use_parent"); - const auto *MatchedLeafStatement = Result.Nodes.getNodeAs<Stmt>("leaf_statement"); +void LostStdMoveCheck::check(const MatchFinder::MatchResult& Result) { + const auto* MatchedDecl = Result.Nodes.getNodeAs<VarDecl>("decl"); + const auto* MatchedFunc = Result.Nodes.getNodeAs<FunctionDecl>("func"); + const auto* MatchedUse = Result.Nodes.getNodeAs<Expr>("use"); + const auto* MatchedUseCall = Result.Nodes.getNodeAs<CallExpr>("use_parent"); + const auto* MatchedLeafStatement = + Result.Nodes.getNodeAs<Stmt>("leaf_statement"); - if (MatchedUseCall) - return; + if (MatchedUseCall) return; - const auto *LastUsage = + const auto* LastUsage = getLastVarUsage(*MatchedDecl, *MatchedFunc, *Result.Context); - if (LastUsage == nullptr) - return; + if (LastUsage == nullptr) return; if (LastUsage->getBeginLoc() > MatchedUse->getBeginLoc()) { // "use" is not the last reference to x @@ -104,15 +101,17 @@ void LostStdMoveCheck::check(const MatchFinder::MatchResult &Result) { } // Calculate X usage count in the statement - llvm::SmallPtrSet<const DeclRefExpr *, 16> DeclRefs; - auto Matches = match(findAll(declRefExpr(to(varDecl(equalsNode(MatchedDecl)))).bind("ref")), *MatchedLeafStatement, *Result.Context); + llvm::SmallPtrSet<const DeclRefExpr*, 16> DeclRefs; + ArrayRef<BoundNodes> Matches = match( + findAll(declRefExpr(to(varDecl(equalsNode(MatchedDecl)))).bind("ref")), + *MatchedLeafStatement, *Result.Context); extractNodesByIdTo(Matches, "ref", DeclRefs); if (DeclRefs.size() > 1) { // Unspecified order of evaluation, e.g. f(x, x) return; } - diag(LastUsage->getBeginLoc(), "Could be std::move()"); + diag(LastUsage->getBeginLoc(), "could be std::move()"); } -} // namespace clang::tidy::performance +} // namespace clang::tidy::performance diff --git a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.h b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.h index c62c3f6448a82..d9bdc1eeffdd7 100644 --- a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.h +++ b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.h @@ -24,12 +24,8 @@ class LostStdMoveCheck : public ClangTidyCheck { 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.CPlusPlus; + return LangOpts.CPlusPlus11; } - -private: - const Expr *getLastVarUsage(const VarDecl &Var, const Decl &Func, - ASTContext &Context); }; } // namespace clang::tidy::performance >From 5359c69e758d6e2f03f19785d4e3354dc81572f5 Mon Sep 17 00:00:00 2001 From: Vasily Kulikov <seg...@yandex-team.ru> Date: Mon, 26 May 2025 15:06:26 +0300 Subject: [PATCH 05/14] auto -> Expr --- clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp index cb6700fca8489..1683584928cde 100644 --- a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp @@ -91,7 +91,7 @@ void LostStdMoveCheck::check(const MatchFinder::MatchResult& Result) { if (MatchedUseCall) return; - const auto* LastUsage = + const Expr* LastUsage = getLastVarUsage(*MatchedDecl, *MatchedFunc, *Result.Context); if (LastUsage == nullptr) return; >From 5bb6af6b3c9f5eef4b662370fec0147ceed2809e Mon Sep 17 00:00:00 2001 From: Vasily Kulikov <seg...@yandex-team.ru> Date: Mon, 26 May 2025 15:19:12 +0300 Subject: [PATCH 06/14] FixIt --- .../clang-tidy/performance/LostStdMoveCheck.cpp | 10 +++++++++- .../clang-tidy/checkers/performance/lost-std-move.cpp | 8 ++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp index 1683584928cde..2f7521a855dff 100644 --- a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp @@ -9,6 +9,7 @@ #include "LostStdMoveCheck.h" #include "../utils/DeclRefExprUtils.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" using namespace clang::ast_matchers; @@ -111,7 +112,14 @@ void LostStdMoveCheck::check(const MatchFinder::MatchResult& Result) { return; } - diag(LastUsage->getBeginLoc(), "could be std::move()"); + + const SourceManager &Source = Result.Context->getSourceManager(); + const auto Range = CharSourceRange::getTokenRange(LastUsage->getSourceRange()); + const StringRef NeedleExprCode = Lexer::getSourceText( + Range, Source, + Result.Context->getLangOpts()); + diag(LastUsage->getBeginLoc(), "could be std::move()") + << FixItHint::CreateReplacement(Range, ("std::move(" + NeedleExprCode + ")").str()); } } // namespace clang::tidy::performance diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp index a4a2550971cac..b88faace6de2e 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp @@ -23,14 +23,14 @@ void f_arg(std::shared_ptr<int> ptr) { if (*ptr) f(ptr); - // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Could be std::move() [performance-lost-std-move] + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: could be std::move() [performance-lost-std-move] } void f_rvalue_ref(std::shared_ptr<int>&& ptr) { if (*ptr) f(ptr); - // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Could be std::move() [performance-lost-std-move] + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: could be std::move() [performance-lost-std-move] } using SharedPtr = std::shared_ptr<int>; @@ -38,7 +38,7 @@ void f_using(SharedPtr ptr) { if (*ptr) f(ptr); - // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Could be std::move() [performance-lost-std-move] + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: could be std::move() [performance-lost-std-move] } void f_local() @@ -46,7 +46,7 @@ void f_local() std::shared_ptr<int> ptr; if (*ptr) f(ptr); - // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Could be std::move() [performance-lost-std-move] + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: could be std::move() [performance-lost-std-move] } void f_move() >From 592d79ba1f4e1bf0b5347565cda50337bbdd3b1e Mon Sep 17 00:00:00 2001 From: Vasily Kulikov <seg...@yandex-team.ru> Date: Mon, 26 May 2025 15:27:49 +0300 Subject: [PATCH 07/14] f((x)) --- .../performance/LostStdMoveCheck.cpp | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp index 2f7521a855dff..e9900b9d15d48 100644 --- a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp @@ -70,7 +70,18 @@ void LostStdMoveCheck::registerMatchers(MatchFinder* Finder) { hasDeclaration( varDecl(hasAncestor(functionDecl().bind("func"))).bind("decl")), - hasParent(expr(hasParent(cxxConstructExpr())).bind("use_parent"))) + anyOf( + + // f(x) + hasParent(expr(hasParent(cxxConstructExpr())).bind("use_parent")), + + // f((x)) + hasParent(parenExpr(hasParent( + expr(hasParent(cxxConstructExpr())).bind("use_parent")))) + + ) + + ) .bind("use"), this); } @@ -112,14 +123,14 @@ void LostStdMoveCheck::check(const MatchFinder::MatchResult& Result) { return; } - - const SourceManager &Source = Result.Context->getSourceManager(); - const auto Range = CharSourceRange::getTokenRange(LastUsage->getSourceRange()); - const StringRef NeedleExprCode = Lexer::getSourceText( - Range, Source, - Result.Context->getLangOpts()); + const SourceManager& Source = Result.Context->getSourceManager(); + const auto Range = + CharSourceRange::getTokenRange(LastUsage->getSourceRange()); + const StringRef NeedleExprCode = + Lexer::getSourceText(Range, Source, Result.Context->getLangOpts()); diag(LastUsage->getBeginLoc(), "could be std::move()") - << FixItHint::CreateReplacement(Range, ("std::move(" + NeedleExprCode + ")").str()); + << FixItHint::CreateReplacement( + Range, ("std::move(" + NeedleExprCode + ")").str()); } } // namespace clang::tidy::performance >From 1710c2f128eb1b1f484923a234fb5bf79ddd0bb4 Mon Sep 17 00:00:00 2001 From: Vasily Kulikov <seg...@yandex-team.ru> Date: Sun, 1 Jun 2025 00:15:17 +0300 Subject: [PATCH 08/14] w --- .../performance/LostStdMoveCheck.cpp | 89 +++++++++++++------ 1 file changed, 64 insertions(+), 25 deletions(-) diff --git a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp index e9900b9d15d48..20efc3af9ed7a 100644 --- a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp @@ -7,7 +7,6 @@ //===----------------------------------------------------------------------===// #include "LostStdMoveCheck.h" -#include "../utils/DeclRefExprUtils.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Lex/Lexer.h" @@ -15,11 +14,33 @@ using namespace clang::ast_matchers; namespace clang::tidy::performance { -using utils::decl_ref_expr::allDeclRefExprs; +template <typename Node> +void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID, + llvm::SmallPtrSet<const Node*, 16>& Nodes) { + for (const BoundNodes& Match : Matches) + Nodes.insert(Match.getNodeAs<Node>(ID)); +} + +llvm::SmallPtrSet<const DeclRefExpr*, 16> allDeclRefExprsHonourLambda( + const VarDecl& VarDecl, const Decl& Decl, ASTContext& Context) { + auto Matches = match( + decl(forEachDescendant( + declRefExpr(to(varDecl(equalsNode(&VarDecl))), + + unless(hasAncestor(lambdaExpr(hasAnyCapture(lambdaCapture( + capturesVar(varDecl(equalsNode(&VarDecl)))))))) + + ) + .bind("declRef"))), + Decl, Context); + llvm::SmallPtrSet<const DeclRefExpr*, 16> DeclRefs; + extractNodesByIdTo(Matches, "declRef", DeclRefs); + return DeclRefs; +} static const Expr* getLastVarUsage(const VarDecl& Var, const Decl& Func, ASTContext& Context) { - auto Exprs = allDeclRefExprs(Var, Func, Context); + auto Exprs = allDeclRefExprsHonourLambda(Var, Func, Context); const Expr* LastExpr = nullptr; for (const clang::DeclRefExpr* Expr : Exprs) { @@ -36,17 +57,16 @@ AST_MATCHER(CXXRecordDecl, hasTrivialMoveConstructor) { } void LostStdMoveCheck::registerMatchers(MatchFinder* Finder) { - auto returnParent = + auto ReturnParent = hasParent(expr(hasParent(cxxConstructExpr(hasParent(returnStmt()))))); - auto outermostExpr = expr(unless(hasParent(expr()))); - auto leafStatement = - stmt(outermostExpr, unless(hasDescendant(outermostExpr))); + auto OutermostExpr = expr(unless(hasParent(expr()))); + auto LeafStatement = stmt(OutermostExpr); Finder->addMatcher( declRefExpr( // not "return x;" - unless(returnParent), + unless(ReturnParent), unless(hasType(namedDecl(hasName("::std::string_view")))), @@ -58,14 +78,17 @@ void LostStdMoveCheck::registerMatchers(MatchFinder* Finder) { hasDeclaration(cxxRecordDecl(hasTrivialMoveConstructor()))))), // Not in a cycle - unless(hasAncestor(forStmt())), unless(hasAncestor(doStmt())), + unless(hasAncestor(forStmt())), + + unless(hasAncestor(doStmt())), + unless(hasAncestor(whileStmt())), // only non-X& unless(hasDeclaration( varDecl(hasType(qualType(lValueReferenceType()))))), - hasAncestor(leafStatement.bind("leaf_statement")), + hasAncestor(LeafStatement.bind("leaf_statement")), hasDeclaration( varDecl(hasAncestor(functionDecl().bind("func"))).bind("decl")), @@ -86,13 +109,6 @@ void LostStdMoveCheck::registerMatchers(MatchFinder* Finder) { this); } -template <typename Node> -void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID, - llvm::SmallPtrSet<const Node*, 16>& Nodes) { - for (const BoundNodes& Match : Matches) - Nodes.insert(Match.getNodeAs<Node>(ID)); -} - void LostStdMoveCheck::check(const MatchFinder::MatchResult& Result) { const auto* MatchedDecl = Result.Nodes.getNodeAs<VarDecl>("decl"); const auto* MatchedFunc = Result.Nodes.getNodeAs<FunctionDecl>("func"); @@ -101,21 +117,35 @@ void LostStdMoveCheck::check(const MatchFinder::MatchResult& Result) { const auto* MatchedLeafStatement = Result.Nodes.getNodeAs<Stmt>("leaf_statement"); - if (MatchedUseCall) return; + if (MatchedUseCall) { + return; + } const Expr* LastUsage = getLastVarUsage(*MatchedDecl, *MatchedFunc, *Result.Context); - if (LastUsage == nullptr) return; - if (LastUsage->getBeginLoc() > MatchedUse->getBeginLoc()) { + if (LastUsage && LastUsage->getBeginLoc() > MatchedUse->getBeginLoc()) { // "use" is not the last reference to x return; } + if (LastUsage && + LastUsage->getSourceRange() != MatchedUse->getSourceRange()) { + return; + } + // Calculate X usage count in the statement llvm::SmallPtrSet<const DeclRefExpr*, 16> DeclRefs; ArrayRef<BoundNodes> Matches = match( - findAll(declRefExpr(to(varDecl(equalsNode(MatchedDecl)))).bind("ref")), + findAll(declRefExpr( + + to(varDecl(equalsNode(MatchedDecl))), + + unless(hasAncestor(lambdaExpr(hasAnyCapture(lambdaCapture( + capturesVar(varDecl(equalsNode(MatchedDecl)))))))) + + ) + .bind("ref")), *MatchedLeafStatement, *Result.Context); extractNodesByIdTo(Matches, "ref", DeclRefs); if (DeclRefs.size() > 1) { @@ -125,12 +155,21 @@ void LostStdMoveCheck::check(const MatchFinder::MatchResult& Result) { const SourceManager& Source = Result.Context->getSourceManager(); const auto Range = - CharSourceRange::getTokenRange(LastUsage->getSourceRange()); + CharSourceRange::getTokenRange(MatchedUse->getSourceRange()); const StringRef NeedleExprCode = Lexer::getSourceText(Range, Source, Result.Context->getLangOpts()); - diag(LastUsage->getBeginLoc(), "could be std::move()") - << FixItHint::CreateReplacement( - Range, ("std::move(" + NeedleExprCode + ")").str()); + + if (NeedleExprCode == "=") { + + diag(MatchedUse->getBeginLoc(), "could be std::move()") + << FixItHint::CreateInsertion( + MatchedUse->getBeginLoc(), + ("std::move(" + MatchedDecl->getName() + "),").str()); + } else { + diag(MatchedUse->getBeginLoc(), "could be std::move()") + << FixItHint::CreateReplacement( + Range, ("std::move(" + NeedleExprCode + ")").str()); + } } } // namespace clang::tidy::performance >From 60e4aca43a3e0a343f4ef327d490b95b8c21da9a Mon Sep 17 00:00:00 2001 From: Vasily Kulikov <seg...@yandex-team.ru> Date: Sun, 1 Jun 2025 00:24:46 +0300 Subject: [PATCH 09/14] thread_local, extern, static --- .../performance/LostStdMoveCheck.cpp | 2 ++ .../checkers/performance/lost-std-move.cpp | 28 +++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp index 20efc3af9ed7a..b9ef7cb9ad61b 100644 --- a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp @@ -117,6 +117,8 @@ void LostStdMoveCheck::check(const MatchFinder::MatchResult& Result) { const auto* MatchedLeafStatement = Result.Nodes.getNodeAs<Stmt>("leaf_statement"); + if (!MatchedDecl->hasLocalStorage()) return; + if (MatchedUseCall) { return; } diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp index b88faace6de2e..a6f267129918c 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp @@ -41,6 +41,27 @@ void f_using(SharedPtr ptr) // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: could be std::move() [performance-lost-std-move] } +void f_thread_local() +{ + thread_local std::shared_ptr<int> ptr; + if (*ptr) + f(ptr); +} + +void f_static() +{ + static std::shared_ptr<int> ptr; + if (*ptr) + f(ptr); +} + +void f_extern() +{ + extern std::shared_ptr<int> ptr; + if (*ptr) + f(ptr); +} + void f_local() { std::shared_ptr<int> ptr; @@ -112,3 +133,10 @@ int f_multiple_usages() std::shared_ptr<int> ptr; return f(ptr) + f(ptr); } + +#define FUN(x) f((x)) +int f_macro() +{ + std::shared_ptr<int> ptr; + return FUN(ptr); +} >From a27b1b301ff10a99b6a8532d439d58be16a53479 Mon Sep 17 00:00:00 2001 From: Vasily Kulikov <seg...@yandex-team.ru> Date: Sun, 1 Jun 2025 00:29:41 +0300 Subject: [PATCH 10/14] better test --- .../checkers/performance/lost-std-move.cpp | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp index a6f267129918c..f6dc90f9a8332 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp @@ -62,6 +62,12 @@ void f_extern() f(ptr); } +std::shared_ptr<int> global; +void f_global() +{ + f(global); +} + void f_local() { std::shared_ptr<int> ptr; @@ -140,3 +146,36 @@ int f_macro() std::shared_ptr<int> ptr; return FUN(ptr); } + +void f_lambda_ref() +{ + std::shared_ptr<int> ptr; + auto Lambda = [&ptr]() mutable { + f(ptr); + }; + Lambda(); +} + +void f_lambda() +{ + std::shared_ptr<int> ptr; + auto Lambda = [ptr]() mutable { + // CHECK-MESSAGES: [[@LINE-1]]:18: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] + // CHECK-FIXES: auto Lambda = [Mov]() mutable { + // Note: No fix, because a fix requires c++14. + f(ptr); + }; + Lambda(); +} + +void f_lambda_assign() +{ + std::shared_ptr<int> ptr; + auto Lambda = [ptr = ptr]() mutable { + // CHECK-MESSAGES: [[@LINE-1]]:18: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] + // CHECK-FIXES: auto Lambda = [Mov]() mutable { + // Note: No fix, because a fix requires c++14. + f(ptr); + }; + Lambda(); +} >From 6aacd41124ebd053a9b3f09cdcd506ffbf49ce7d Mon Sep 17 00:00:00 2001 From: Vasily Kulikov <seg...@yandex-team.ru> Date: Sun, 1 Jun 2025 07:52:37 +0300 Subject: [PATCH 11/14] fixes --- .../clang-tidy/performance/LostStdMoveCheck.cpp | 5 ++++- .../checkers/performance/lost-std-move.cpp | 17 ++++++++++------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp index b9ef7cb9ad61b..47188ea87fc17 100644 --- a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp @@ -21,7 +21,7 @@ void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID, Nodes.insert(Match.getNodeAs<Node>(ID)); } -llvm::SmallPtrSet<const DeclRefExpr*, 16> allDeclRefExprsHonourLambda( +static llvm::SmallPtrSet<const DeclRefExpr*, 16> allDeclRefExprsHonourLambda( const VarDecl& VarDecl, const Decl& Decl, ASTContext& Context) { auto Matches = match( decl(forEachDescendant( @@ -84,6 +84,9 @@ void LostStdMoveCheck::registerMatchers(MatchFinder* Finder) { unless(hasAncestor(whileStmt())), + // Not in a body of lambda + unless(hasAncestor(compoundStmt(hasAncestor(lambdaExpr())))), + // only non-X& unless(hasDeclaration( varDecl(hasType(qualType(lValueReferenceType()))))), diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp index f6dc90f9a8332..23693e578ef1b 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp @@ -24,6 +24,7 @@ void f_arg(std::shared_ptr<int> ptr) if (*ptr) f(ptr); // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: could be std::move() [performance-lost-std-move] + // CHECK-FIXES: f(std::move(ptr)); } void f_rvalue_ref(std::shared_ptr<int>&& ptr) @@ -31,6 +32,7 @@ void f_rvalue_ref(std::shared_ptr<int>&& ptr) if (*ptr) f(ptr); // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: could be std::move() [performance-lost-std-move] + // CHECK-FIXES: f(std::move(ptr)); } using SharedPtr = std::shared_ptr<int>; @@ -74,6 +76,7 @@ void f_local() if (*ptr) f(ptr); // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: could be std::move() [performance-lost-std-move] + // CHECK-FIXES: f(std::move(ptr)); } void f_move() @@ -131,7 +134,7 @@ void f_cycle4() std::shared_ptr<int> ptr; do { f(ptr); - } while (*ptr); + } while (true); } int f_multiple_usages() @@ -145,6 +148,8 @@ int f_macro() { std::shared_ptr<int> ptr; return FUN(ptr); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: could be std::move() [performance-lost-std-move] + // CHECK-FIXES: return FUN(std::move(ptr)); } void f_lambda_ref() @@ -160,9 +165,8 @@ void f_lambda() { std::shared_ptr<int> ptr; auto Lambda = [ptr]() mutable { - // CHECK-MESSAGES: [[@LINE-1]]:18: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] - // CHECK-FIXES: auto Lambda = [Mov]() mutable { - // Note: No fix, because a fix requires c++14. + // CHECK-MESSAGES: [[@LINE-1]]:18: warning: could be std::move() [performance-lost-std-move] + // CHECK-FIXES: auto Lambda = [std::move(ptr)]() mutable { f(ptr); }; Lambda(); @@ -172,9 +176,8 @@ void f_lambda_assign() { std::shared_ptr<int> ptr; auto Lambda = [ptr = ptr]() mutable { - // CHECK-MESSAGES: [[@LINE-1]]:18: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use] - // CHECK-FIXES: auto Lambda = [Mov]() mutable { - // Note: No fix, because a fix requires c++14. + // CHECK-MESSAGES: [[@LINE-1]]:24: warning: could be std::move() [performance-lost-std-move] + // CHECK-FIXES: auto Lambda = [ptr = std::move(ptr)]() mutable { f(ptr); }; Lambda(); >From 71cd708c34203f4caa7b97b8efa15d98ce1867c4 Mon Sep 17 00:00:00 2001 From: Vasily Kulikov <seg...@yandex-team.ru> Date: Sun, 1 Jun 2025 08:02:12 +0300 Subject: [PATCH 12/14] [=] test --- .../clang-tidy/performance/LostStdMoveCheck.cpp | 2 +- .../clang-tidy/checkers/performance/lost-std-move.cpp | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp index 47188ea87fc17..9699e66023c48 100644 --- a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp @@ -169,7 +169,7 @@ void LostStdMoveCheck::check(const MatchFinder::MatchResult& Result) { diag(MatchedUse->getBeginLoc(), "could be std::move()") << FixItHint::CreateInsertion( MatchedUse->getBeginLoc(), - ("std::move(" + MatchedDecl->getName() + "),").str()); + (MatchedDecl->getName() + " = std::move(" + MatchedDecl->getName() + "),").str()); } else { diag(MatchedUse->getBeginLoc(), "could be std::move()") << FixItHint::CreateReplacement( diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp index 23693e578ef1b..a346f6e4a3f65 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp @@ -182,3 +182,14 @@ void f_lambda_assign() }; Lambda(); } + +void f_lambda_assign_all() +{ + std::shared_ptr<int> ptr; + auto Lambda = [=]() mutable { + // CHECK-MESSAGES: [[@LINE-1]]:18: warning: could be std::move() [performance-lost-std-move] + // CHECK-FIXES: auto Lambda = [ptr = std::move(ptr),=]() mutable { + f(ptr); + }; + Lambda(); +} >From 402ba55ed1b536c256c0f08b78a4dceb405a187d Mon Sep 17 00:00:00 2001 From: Vasily Kulikov <seg...@yandex-team.ru> Date: Sun, 1 Jun 2025 08:03:55 +0300 Subject: [PATCH 13/14] fix --- clang-tools-extra/docs/ReleaseNotes.rst | 67 ------------------------- 1 file changed, 67 deletions(-) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 2f5af851fcbac..e0f81a032c38d 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -145,75 +145,8 @@ New checks - New :doc:`readability-ambiguous-smartptr-reset-call <clang-tidy/checks/readability/ambiguous-smartptr-reset-call>` check. -<<<<<<< HEAD - Detects implicit conversions between pointers of different levels of - indirection. - -- New :doc:`bugprone-optional-value-conversion - <clang-tidy/checks/bugprone/optional-value-conversion>` check. - - Detects potentially unintentional and redundant conversions where a value is - extracted from an optional-like type and then used to create a new instance - of the same optional-like type. - -- New :doc:`cppcoreguidelines-no-suspend-with-lock - <clang-tidy/checks/cppcoreguidelines/no-suspend-with-lock>` check. - - Flags coroutines that suspend while a lock guard is in scope at the - suspension point. - -- New :doc:`hicpp-ignored-remove-result - <clang-tidy/checks/hicpp/ignored-remove-result>` check. - - Ensure that the result of ``std::remove``, ``std::remove_if`` and - ``std::unique`` are not ignored according to rule 17.5.1. - -- New :doc:`misc-coroutine-hostile-raii - <clang-tidy/checks/misc/coroutine-hostile-raii>` check. - - Detects when objects of certain hostile RAII types persists across suspension - points in a coroutine. Such hostile types include scoped-lockable types and - types belonging to a configurable denylist. - -- New :doc:`modernize-use-constraints - <clang-tidy/checks/modernize/use-constraints>` check. - - Replace ``enable_if`` with C++20 requires clauses. - -- New :doc:`modernize-use-starts-ends-with - <clang-tidy/checks/modernize/use-starts-ends-with>` check. - - Checks whether a ``find`` or ``rfind`` result is compared with 0 and suggests - replacing with ``starts_with`` when the method exists in the class. Notably, - this will work with ``std::string`` and ``std::string_view``. - -- New :doc:`modernize-use-std-numbers - <clang-tidy/checks/modernize/use-std-numbers>` check. - - Finds constants and function calls to math functions that can be replaced - with C++20's mathematical constants from the ``numbers`` header and - offers fix-it hints. - -- New :doc:`performance-enum-size - <clang-tidy/checks/performance/enum-size>` check. - - Recommends the smallest possible underlying type for an ``enum`` or ``enum`` - class based on the range of its enumerators. - -- New :doc:`performance-lost-std-move - <clang-tidy/checks/performance/lost-std-move>` check. - - Searches for lost std::move(). - -- New :doc:`readability-reference-to-constructed-temporary - <clang-tidy/checks/readability/reference-to-constructed-temporary>` check. - - Detects C++ code where a reference variable is used to extend the lifetime - of a temporary object that has just been constructed. -======= Finds potentially erroneous calls to ``reset`` method on smart pointers when the pointee type also has a ``reset`` method. ->>>>>>> origin/main New check aliases ^^^^^^^^^^^^^^^^^ >From fdf01b62a2b7619c5d3e12700c9d756296479e62 Mon Sep 17 00:00:00 2001 From: Vasily Kulikov <seg...@yandex-team.ru> Date: Sun, 1 Jun 2025 08:14:23 +0300 Subject: [PATCH 14/14] docs --- .../clang-tidy/performance/LostStdMoveCheck.h | 3 ++- clang-tools-extra/docs/ReleaseNotes.rst | 6 ++++++ .../checks/performance/lost-std-move.rst | 20 ++++++++++++++++++- 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.h b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.h index d9bdc1eeffdd7..789f1233e7042 100644 --- a/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.h +++ b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.h @@ -13,7 +13,8 @@ namespace clang::tidy::performance { -/// Search for lost std::move(). +/// Warns if copy constructor is used instead of std::move() and suggests a fix. +/// It honours cycles, lambdas, and unspecified call order in compound expressions. /// /// For the user-facing documentation see: /// http://clang.llvm.org/extra/clang-tidy/checks/performance/lost-std-move.html diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index e0f81a032c38d..7f5e01b1fba1f 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -136,6 +136,12 @@ New checks Finds unintended character output from ``unsigned char`` and ``signed char`` to an ``ostream``. +- New :doc:`performance-lost-std-move + <clang-tidy/checks/performance/lost-std-move>` check. + + Warns if copy constructor is used instead of std::move() and suggests a fix. + It honours cycles, lambdas, and unspecified call order in compound expressions. + - New :doc:`portability-avoid-pragma-once <clang-tidy/checks/portability/avoid-pragma-once>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/lost-std-move.rst b/clang-tools-extra/docs/clang-tidy/checks/performance/lost-std-move.rst index ded49de7b8126..13cf7b63440f7 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/performance/lost-std-move.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/performance/lost-std-move.rst @@ -3,7 +3,8 @@ performance-lost-std-move ========================= -The check warns if copy constructor is used instead of std::move(). +Warns if copy constructor is used instead of std::move() and suggests a fix. +It honours cycles, lambdas, and unspecified call order in compound expressions. .. code-block:: c++ @@ -12,3 +13,20 @@ The check warns if copy constructor is used instead of std::move(). void g(X x) { f(x); // warning: Could be std::move() [performance-lost-std-move] } + +It finds the last local variable usage, and if it is a copy, emits a warning. +The check is based on pure AST matching and doesn't take into account any data flow information. +Thus, it doesn't catch assign-after-copy cases. +Also it doesn't notice variable references "behind the scenes": + +.. code-block:: c++ + + void f(X); + + void g(X x) { + auto &y = x; + f(x); // emits a warning... + y.f(); // ...but it is still used + } + +Such rare cases should be silenced using `// NOLINT`. _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
