diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt index c6e547c5089fb..e16a5fdbaa60f 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 STATIC 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..0746555cd7dd7 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.cpp @@ -0,0 +1,196 @@ +//===----------------------------------------------------------------------===// +// +// 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 "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::performance { + +template +static void extractNodesByIdTo(ArrayRef Matches, StringRef ID, + llvm::SmallPtrSet &Nodes) { + for (const BoundNodes &Match : Matches) + Nodes.insert(Match.getNodeAs(ID)); +} + +static llvm::SmallPtrSet +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 DeclRefs; + extractNodesByIdTo(Matches, "declRef", DeclRefs); + return DeclRefs; +} + +static llvm::SmallPtrSet +allVarDeclsExprs(const VarDecl &VarDecl, const Decl &Decl, + ASTContext &Context) { + auto Matches = match( + decl(forEachDescendant(declRefExpr( + to(varDecl(equalsNode(&VarDecl))), + hasParent(decl( + varDecl(hasType(qualType(referenceType()))).bind("varDecl"))), + unless(hasAncestor(lambdaExpr(hasAnyCapture( + lambdaCapture(capturesVar(varDecl(equalsNode(&VarDecl))))))))))), + Decl, Context); + llvm::SmallPtrSet VarDecls; + extractNodesByIdTo(Matches, "varDecl", VarDecls); + return VarDecls; +} + +static const Expr * +getLastVarUsage(const llvm::SmallPtrSet &Exprs) { + const Expr *LastExpr = nullptr; + for (const clang::DeclRefExpr *Expr : Exprs) { + if (!LastExpr) + LastExpr = Expr; + + if (LastExpr->getBeginLoc() < Expr->getBeginLoc()) + LastExpr = Expr; + } + + return LastExpr; +} + +namespace { + +AST_MATCHER(CXXRecordDecl, hasTrivialMoveConstructor) { + return Node.hasDefinition() && Node.hasTrivialMoveConstructor(); +} + +AST_MATCHER_P(Expr, ignoreParens, ast_matchers::internal::Matcher, + InnerMatcher) { + return InnerMatcher.matches(*Node.IgnoreParens(), Finder, Builder); +} + +} // namespace + +void LostStdMoveCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "StrictMode", StrictMode); +} + +LostStdMoveCheck::LostStdMoveCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + StrictMode(Options.get("StrictMode", false)) {} + +void LostStdMoveCheck::registerMatchers(MatchFinder *Finder) { + auto ReturnParent = + hasParent(expr(hasParent(cxxConstructExpr(hasParent(returnStmt()))))); + + auto OutermostExpr = expr(unless(hasParent(expr()))); + auto LeafStatement = stmt(OutermostExpr); + + Finder->addMatcher( + cxxConstructExpr(has(expr(has(ignoreParens( + 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())), + // Not in a body of lambda + unless(hasAncestor(compoundStmt(hasAncestor(lambdaExpr())))), + // only non-X& + unless(hasDeclaration( + varDecl(hasType(qualType(lValueReferenceType()))))), + hasAncestor(LeafStatement.bind("leaf_statement")), + hasDeclaration(varDecl(hasAncestor(functionDecl().bind("func"))) + .bind("decl"))) + .bind("use")))))), + this); +} + +void LostStdMoveCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MatchedDecl = Result.Nodes.getNodeAs("decl"); + const auto *MatchedFunc = Result.Nodes.getNodeAs("func"); + const auto *MatchedUse = Result.Nodes.getNodeAs("use"); + const auto *MatchedLeafStatement = + Result.Nodes.getNodeAs("leaf_statement"); + + if (!MatchedDecl->hasLocalStorage()) { + // global or static variable + return; + } + + if (!StrictMode) { + llvm::SmallPtrSet AllVarDecls = + allVarDeclsExprs(*MatchedDecl, *MatchedFunc, *Result.Context); + if (!AllVarDecls.empty()) { + // x is referenced by local var, it may outlive the "use" + return; + } + } + + llvm::SmallPtrSet AllReferences = + allDeclRefExprsHonourLambda(*MatchedDecl, *MatchedFunc, *Result.Context); + const Expr *LastUsage = getLastVarUsage(AllReferences); + + 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 DeclRefs; + + SmallVector Matches = match( + 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) { + // Unspecified order of evaluation, e.g. f(x, x) + return; + } + + const SourceManager &Source = Result.Context->getSourceManager(); + const auto Range = + CharSourceRange::getTokenRange(MatchedUse->getSourceRange()); + const StringRef NeedleExprCode = + Lexer::getSourceText(Range, Source, Result.Context->getLangOpts()); + + if (NeedleExprCode == "=") { + diag(MatchedUse->getBeginLoc(), "could be std::move()") + << FixItHint::CreateInsertion(MatchedUse->getBeginLoc(), + (MatchedDecl->getName() + + " = std::move(" + + MatchedDecl->getName() + "),") + .str()); + } else { + diag(MatchedUse->getBeginLoc(), "could be std::move()") + << FixItHint::CreateReplacement( + Range, ("std::move(" + NeedleExprCode + ")").str()); + } +} + +} // 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..d821d4a05ad62 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/LostStdMoveCheck.h @@ -0,0 +1,38 @@ +//===----------------------------------------------------------------------===// +// +// 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 { + +/// 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: +/// https://clang.llvm.org/extra/clang-tidy/checks/performance/lost-std-move.html +class LostStdMoveCheck : public ClangTidyCheck { +public: + LostStdMoveCheck(StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus11; + } + +private: + const bool StrictMode; +}; + +} // 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 ae15208ae3dc5..ed9ccbdd76646 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( "performance-inefficient-vector-operation"); + CheckFactories.registerCheck("performance-lost-std-move"); CheckFactories.registerCheck( "performance-move-const-arg"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 62e1987377989..3f09c2a6e61a9 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -203,6 +203,11 @@ New checks Finds virtual function overrides with different visibility than the function in the base class. +- New :doc:`performance-lost-std-move + ` check. + + Warns if copy constructor is used instead of ``std::move()`` and suggests a fix. + - New :doc:`readability-redundant-parentheses ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index f94696d4ef9c7..818d5ab354a51 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -350,6 +350,7 @@ Clang-Tidy Checks :doc:`performance-inefficient-algorithm `, "Yes" :doc:`performance-inefficient-string-concatenation `, :doc:`performance-inefficient-vector-operation `, "Yes" + :doc:`performance-lost-std-move `, "Yes" :doc:`performance-move-const-arg `, "Yes" :doc:`performance-move-constructor-init `, :doc:`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..fd625eec8117c --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/performance/lost-std-move.rst @@ -0,0 +1,48 @@ +.. title:: clang-tidy - performance-lost-std-move + +performance-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. + +.. code-block:: c++ + + void f(X); + + 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 does notice variable references "behind the scenes": + +.. code-block:: c++ + + void f(X); + + void g(X x) { + auto &y = x; + f(x); // does not emit a warning + y.f(); // because x is still used via y + } + +If you want to ignore assigns to reference variables, set :option:`StrictMode` +to `true`. + + +Options +------- + +.. option:: StrictMode + + A variable ``X`` can be referenced by another variable ``R``. In this case + the last variable usage might be not from ``X``, but from ``R``. It is quite + difficult to find in a large function, so if the plugin sees some ``R`` + 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`. diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move-strict.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move-strict.cpp new file mode 100644 index 0000000000000..cdaa29cd25ebd --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move-strict.cpp @@ -0,0 +1,30 @@ +// RUN: %check_clang_tidy %s performance-lost-std-move %t -- -config='{CheckOptions: {performance-lost-std-move.StrictMode: true}}' + +namespace std { + +template +class shared_ptr { +public: + T& operator*() { return reinterpret_cast(*this); } + shared_ptr() {} + shared_ptr(const shared_ptr&) {} +}; + +template +T&& move(T&) +{ +} + +} // namespace std + +int f(std::shared_ptr); + +void f_copy_after_ref() +{ + std::shared_ptr ptr; + auto& ref = ptr; + f(ptr); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: could be std::move() [performance-lost-std-move] + // CHECK-FIXES: f(std::move(ptr)); + *ref = 1; +} 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..c0b35a5160414 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/lost-std-move.cpp @@ -0,0 +1,216 @@ +// RUN: %check_clang_tidy %s performance-lost-std-move %t + +namespace std { + +template +class shared_ptr { +public: + T& operator*() { return reinterpret_cast(*this); } + shared_ptr() {} + shared_ptr(const shared_ptr&) {} +}; + +template +T&& move(T&) +{ +} + +} // namespace std + +int f(std::shared_ptr); + +void f_arg(std::shared_ptr 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&& 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; +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_thread_local() +{ + thread_local std::shared_ptr ptr; + if (*ptr) + f(ptr); +} + +void f_static() +{ + static std::shared_ptr ptr; + if (*ptr) + f(ptr); +} + +void f_extern() +{ + extern std::shared_ptr ptr; + if (*ptr) + f(ptr); +} + +std::shared_ptr global; +void f_global() +{ + f(global); +} + +void f_local() +{ + std::shared_ptr 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_move() +{ + std::shared_ptr ptr; + if (*ptr) + f(std::move(ptr)); +} + +void f_ref(std::shared_ptr &ptr) +{ + if (*ptr) + f(ptr); +} + +std::shared_ptr f_return() +{ + std::shared_ptr ptr; + return ptr; +} + +void f_still_used(std::shared_ptr ptr) +{ + if (*ptr) + f(ptr); + + *ptr = 1; + *ptr = *ptr; +} + +void f_cycle1() +{ + std::shared_ptr ptr; + for(;;) + f(ptr); +} + +void f_cycle2() +{ + std::shared_ptr ptr; + for(int i=0; i<5; i++) + f(ptr); +} + +void f_cycle3() +{ + std::shared_ptr ptr; + while (*ptr) { + f(ptr); + } +} + +void f_cycle4() +{ + std::shared_ptr ptr; + do { + f(ptr); + } while (true); +} + +int f_multiple_usages() +{ + std::shared_ptr ptr; + return f(ptr) + f(ptr); +} + +#define FUN(x) f((x)) +int f_macro() +{ + std::shared_ptr 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() +{ + std::shared_ptr ptr; + auto Lambda = [&ptr]() mutable { + f(ptr); + }; + Lambda(); +} + +void f_lambda() +{ + std::shared_ptr ptr; + auto Lambda = [ptr]() mutable { + // 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(); +} + +void f_lambda_assign() +{ + std::shared_ptr ptr; + auto Lambda = [ptr = ptr]() mutable { + // 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(); +} + +void f_lambda_assign_all() +{ + std::shared_ptr 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(); +} + +void f_copy_after_ref() +{ + std::shared_ptr ptr; + auto& ref = ptr; + f(ptr); + *ref = 1; +} + +int f_lvalue(std::shared_ptr&); +int f_lvalue_const(const std::shared_ptr&); + +void f_ref_lvalue(std::shared_ptr ptr) +{ + f_lvalue(ptr); // no fix +} + +void f_ref_lvalue_const(std::shared_ptr ptr) +{ + f_lvalue_const(ptr); // no fix +}