[clang-tidy] Add check performance-lost-std-move #139525
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-clang-tools-extra Author: Vasiliy Kulikov (segoon) ChangesFull diff: https://github.com/llvm/llvm-project/pull/139525.diff 8 Files Affected:
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);
+}
|
Member
|
@llvm/pr-subscribers-clang-tidy Author: Vasiliy Kulikov (segoon) ChangesFull diff: https://github.com/llvm/llvm-project/pull/139525.diff 8 Files Affected:
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);
+}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Contributor
There was a problem hiding this comment.
Please do not use auto unless type is explicitly states in same statement or iterator.
Contributor
There was a problem hiding this comment.
Suggested change
| Searches for lost std::move(). | |
| Searches for lost ``std::move()``. |
Contributor
There was a problem hiding this comment.
Please synchronize with statement in Release Notes. This check should be omitted.
vbvictor
reviewed
May 21, 2025
Contributor
vbvictor
left a comment
vbvictor
left a comment
There was a problem hiding this comment.
Commented general suggestions/improvements for the check without deep dive into matchers/tests.
Please look at tests in previous attempt in creating such check https://reviews.llvm.org/D137205.
Check that all test cases from that PR are covered in your tests.
If some cases are not covered and you can't address them, please add them to Limitations in docs and leave a fixme.
Contributor
There was a problem hiding this comment.
Suggested change
| return LangOpts.CPlusPlus; | |
| return LangOpts.CPlusPlus11; |
Contributor
There was a problem hiding this comment.
this should be converted to static function and move to .cpp file if no fields of check class are needed
Contributor
There was a problem hiding this comment.
Use explicit type instead of auto if it isn't written explicitly in code line.
Contributor
There was a problem hiding this comment.
Try to provide fixits with the insertion of std::move
Contributor
There was a problem hiding this comment.
This should be synced with release notes and check docs
Contributor
There was a problem hiding this comment.
This need a little more description when the check will warn, e.g when this will fire, with what types?
Look for reference at https://reviews.llvm.org/D137205
Contributor
There was a problem hiding this comment.
Nit suggestion: try make the description similar to already existing new checks in ReleaseNotes.rst.
Description should start with "Finds ..."
Contributor
There was a problem hiding this comment.
Give examples when check will not warn
segoon
force-pushed
the
feature/lost-std-move
branch
from
d3b8c17 to
6aacd41
Compare
| const auto* MatchedLeafStatement = | ||
| Result.Nodes.getNodeAs<Stmt>("leaf_statement"); | ||
|
|
||
| if (!MatchedDecl->hasLocalStorage()) return; |
Contributor
There was a problem hiding this comment.
Should be two lines as next statement.
| - New :doc:`readability-ambiguous-smartptr-reset-call | ||
| <clang-tidy/checks/readability/ambiguous-smartptr-reset-call>` check. | ||
|
|
||
| <<<<<<< HEAD |
Comment on lines
33
to
34
| If you want to ignore assigns to reference variables, set ``StrictMode`` | ||
| to ``false``. |
Contributor
There was a problem hiding this comment.
Suggested change
| If you want to ignore assigns to reference variables, set ``StrictMode`` | |
| to ``false``. | |
| If you want to ignore assigns to reference variables, set :option:`StrictMode` | |
| to `false`. |
Comment on lines
33
to
34
| If you want to ignore assigns to reference variables, set ::option:: | ||
| `StrictMode` to `true`. |
Contributor
There was a problem hiding this comment.
Suggested change
| If you want to ignore assigns to reference variables, set ::option:: | |
| `StrictMode` to `true`. | |
| If you want to ignore assigns to reference variables, set :option:`StrictMode` | |
| to `true`. |
| y.f(); // because x is still used via y | ||
| } | ||
|
|
||
| If you want to ignore assigns to reference variables, set :option: `StrictMode` |
Contributor
There was a problem hiding this comment.
Suggested change
| If you want to ignore assigns to reference variables, set :option: `StrictMode` | |
| If you want to ignore assigns to reference variables, set :option:`StrictMode` |
This is statement. See :program: in clang-tools-extra/docs/ReleaseNotes.rst as example.
Contributor
Author
There was a problem hiding this comment.
thanks! sorry for so many small typos :(
| references ``X``, then it will not emit a warning for ``X`` not to provoke | ||
| false positive. If you're sure that such references don't extend ``X`` | ||
| lifetime and ready to handle possible false positives, then set `StrictMode` | ||
| to `true`. Defaults to `false`. |
Contributor
There was a problem hiding this comment.
Suggested change
| to `true`. Defaults to `false`. | |
| to `true`. Default is `false`. |
clang-tools-extra/docs/clang-tidy/checks/performance/lost-std-move.rst
Outdated
Show resolved
Hide resolved
…move.rst Co-authored-by: EugeneZelenko <[email protected]>
| } | ||
|
|
||
| const SourceManager &Source = Result.Context->getSourceManager(); | ||
| const auto Range = |
Contributor
There was a problem hiding this comment.
auto should not be used - type is not spelled explicitly or iterator.
| - 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. |
Contributor
There was a problem hiding this comment.
Suggested change
| Warns if copy constructor is used instead of ``std::move()`` and suggests a fix. | |
| Warns if copy constructor is used instead of ``std::move()`` and suggests a | |
| fix. |
Contributor
Author
|
@EugeneZelenko hi, any progress here? should I fix/add something else? |
Contributor
You need to wait for other reviewers. I mostly check documentation and minor code style issues. |
Contributor
Author
|
@vbvictor @HerrCai0907 @Febbe Hi! You gave me valuable feedback, thank you! Do you have other comments? What are my next steps? |
vbvictor
reviewed
Nov 11, 2025
Contributor
vbvictor
left a comment
vbvictor
left a comment
There was a problem hiding this comment.
Sorry for missing out on this!
Left initiall feedback, I will soon try to look closely.
For now it just fells that this complex check is under-tested, we better think about more potential edge cases.
| declRefExpr( | ||
| // not "return x;" | ||
| unless(ReturnParent), | ||
| unless(hasType(namedDecl(hasName("::std::string_view")))), |
Contributor
There was a problem hiding this comment.
What about other non-owning containers? I think we need an option for it.
At least people with c++11/c++14 use boost::string_view
Comment on lines
+17
to
+22
| template <typename Node> | ||
| static void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID, | ||
| llvm::SmallPtrSet<const Node *, 16> &Nodes) { | ||
| for (const BoundNodes &Match : Matches) | ||
| Nodes.insert(Match.getNodeAs<Node>(ID)); | ||
| } |
Contributor
There was a problem hiding this comment.
Instead of copying from clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp, could we expose extractNodesByIdTo as public function and reuse here
Comment on lines
+103
to
+104
| // non-trivial type | ||
| hasType(hasCanonicalType(hasDeclaration(cxxRecordDecl()))), |
Contributor
There was a problem hiding this comment.
Do we want to just include RecordDecls or check constructors is not expensive?
We have
llvm-project/clang-tools-extra/clang-tidy/utils/Matchers.h
Lines 25 to 29 in b4c4013
or
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/utils/Matchers.h#L31-L34
Comment on lines
+91
to
+92
| auto ReturnParent = | ||
| hasParent(expr(hasParent(cxxConstructExpr(hasParent(returnStmt()))))); |
Contributor
There was a problem hiding this comment.
Could we just use hasAncestor?
| auto ReturnParent = | ||
| hasParent(expr(hasParent(cxxConstructExpr(hasParent(returnStmt()))))); | ||
|
|
||
| auto OutermostExpr = expr(unless(hasParent(expr()))); |
Comment on lines
+130
to
+133
| if (!MatchedDecl->hasLocalStorage()) { | ||
| // global or static variable | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
We can check for local storage in matchers
| return; | ||
| } | ||
|
|
||
| // Calculate X usage count in the statement |
Contributor
There was a problem hiding this comment.
Should be number of usage?
Comment on lines
+153
to
+156
| if (LastUsage && | ||
| LastUsage->getSourceRange() != MatchedUse->getSourceRange()) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
If we write such condition, does it matter to check for LastUsage->getBeginLoc() > MatchedUse->getBeginLoc() above?
Source range is begin loc + end loc so if any of the locations are mismatched - we return. This case must cover the previous case, or I'm missing something?
Contributor
There was a problem hiding this comment.
And why do we check this? should it be == instead to check that be don't have last usage equal to current matched variable?
Comment on lines
+150
to
+152
| return FUN(ptr); | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: could be std::move() [performance-lost-std-move] | ||
| // CHECK-FIXES: return FUN(std::move(ptr)); |
Contributor
There was a problem hiding this comment.
I don't this is desired behavior.
What if macros comes from 3rd party lib that we can not change?
We can ether ignore all macros or add an option for it.
| void f_ref_lvalue_const(std::shared_ptr<int> ptr) | ||
| { | ||
| f_lvalue_const(ptr); // no fix | ||
| } |
Contributor
There was a problem hiding this comment.
Could we add tests for build-in types (int) and also types with deleted move constructors/assign operators?
Also tests in temlate functions with template variables.
What will happen if one function is instantiated with movable type and other with non-movable? Will the check fire?
Contributor
|
Did you try running this check on some codebases? The easiest way is to run it on I found such case: The check doesn't consider already Also, there are many "false positives" where the check suggest to use I think we should omit these cases reported by
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.