diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt @@ -9,6 +9,7 @@ InefficientVectorOperationCheck.cpp MoveConstArgCheck.cpp MoveConstructorInitCheck.cpp + NoAutomaticMoveCheck.cpp NoexceptMoveConstructorCheck.cpp PerformanceTidyModule.cpp TriviallyDestructibleCheck.cpp diff --git a/clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.h b/clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.h @@ -0,0 +1,36 @@ +//===--- NoAutomaticMoveCheck.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_NOAUTOMATICMOVECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_NOAUTOMATICMOVECHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace performance { + +/// Finds local variables that cannot be automatically moved due to constness. +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/performance-no-automatic-move.html +class NoAutomaticMoveCheck : public ClangTidyCheck { +public: + NoAutomaticMoveCheck(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; + +private: + const std::vector AllowedTypes; +}; + +} // namespace performance +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_NOAUTOMATICMOVECHECK_H diff --git a/clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp @@ -0,0 +1,74 @@ +//===--- NoAutomaticMoveCheck.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 "NoAutomaticMoveCheck.h" +#include "../utils/Matchers.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace performance { + +NoAutomaticMoveCheck::NoAutomaticMoveCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + AllowedTypes( + utils::options::parseStringList(Options.get("AllowedTypes", ""))) {} + +void NoAutomaticMoveCheck::registerMatchers(MatchFinder *Finder) { + // Automatic move exists only for c++11 onwards. + if (!getLangOpts().CPlusPlus11) + return; + + const auto ConstLocalVariable = + varDecl(hasLocalStorage(), unless(hasType(lValueReferenceType())), + hasType(qualType( + isConstQualified(), + hasCanonicalType(matchers::isExpensiveToCopy()), + unless(hasDeclaration(namedDecl( + matchers::matchesAnyListedName(AllowedTypes))))))) + .bind("vardecl"); + + // A matcher for a `DstT::DstT(const Src&)` where DstT also has a + // `DstT::DstT(Src&&)`. + const auto LValueRefCtor = cxxConstructorDecl( + hasParameter(0, + hasType(lValueReferenceType(pointee(type().bind("SrcT"))))), + ofClass(cxxRecordDecl(hasMethod(cxxConstructorDecl( + hasParameter(0, hasType(rValueReferenceType( + pointee(type(equalsBoundNode("SrcT"))))))))))); + + Finder->addMatcher( + returnStmt( + hasReturnValue(ignoringElidableConstructorCall(ignoringParenImpCasts( + cxxConstructExpr(hasDeclaration(LValueRefCtor), + hasArgument(0, ignoringParenImpCasts(declRefExpr( + to(ConstLocalVariable))))) + .bind("ctor_call"))))), + this); +} + +void NoAutomaticMoveCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Var = Result.Nodes.getNodeAs("vardecl"); + const auto *CtorCall = Result.Nodes.getNodeAs("ctor_call"); + diag(CtorCall->getExprLoc(), "constness of '%0' prevents automatic move") + << Var->getName(); +} + +void NoAutomaticMoveCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "AllowedTypes", + utils::options::serializeStringList(AllowedTypes)); +} + +} // namespace performance +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp --- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp @@ -17,6 +17,7 @@ #include "InefficientVectorOperationCheck.h" #include "MoveConstArgCheck.h" #include "MoveConstructorInitCheck.h" +#include "NoAutomaticMoveCheck.h" #include "NoexceptMoveConstructorCheck.h" #include "TriviallyDestructibleCheck.h" #include "TypePromotionInMathFnCheck.h" @@ -46,6 +47,8 @@ "performance-move-const-arg"); CheckFactories.registerCheck( "performance-move-constructor-init"); + CheckFactories.registerCheck( + "performance-no-automatic-move"); CheckFactories.registerCheck( "performance-noexcept-move-constructor"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -143,6 +143,11 @@ Finds Objective-C implementations that implement ``-isEqual:`` without also appropriately implementing ``-hash``. +- New :doc:`performance-no-automatic-move + ` check. + + Finds local variables that cannot be automatically moved due to constness. + - New :doc:`performance-trivially-destructible ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -344,6 +344,7 @@ performance-inefficient-vector-operation performance-move-const-arg performance-move-constructor-init + performance-no-automatic-move performance-noexcept-move-constructor performance-trivially-destructible performance-type-promotion-in-math-fn diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst b/clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst @@ -0,0 +1,53 @@ +.. title:: clang-tidy - performance-no-automatic-move + +performance-no-automatic-move +============================= + +Finds local variables that cannot be automatically moved due to constness. + +Under +`certain conditions `_, +local values are automatically moved out when returning from a function. A +common mistake is to declare local ``lvalue`` variables ``const``, which +prevents the move. + +Example `[1] `_: + +.. code-block:: c++ + + StatusOr> Cool() { + std::vector obj = ...; + return obj; // calls StatusOr::StatusOr(std::vector&&) + } + + StatusOr> NotCool() { + const std::vector obj = ...; + return obj; // calls `StatusOr::StatusOr(const std::vector&)` + } + +The former version (``Cool``) should be preferred over the latter (``Uncool``) +as it will avoid allocations and potentially large memory copies. + +Semantics +--------- + +In the example above, ``StatusOr::StatusOr(T&&)`` have the same semantics as +long as the copy and move constructors for ``T`` have the same semantics. Note +that there is no guarantee that ``S::S(T&&)`` and ``S::S(const T&)`` have the +same semantics for any single ``S``, so we're not providing automated fixes for +this check, and judgement should be exerted when making the suggested changes. + +-Wreturn-std-move +----------------- + +Another case where the move cannot happen is the following: + +.. code-block:: c++ + + StatusOr> Uncool() { + std::vector&& obj = ...; + return obj; // calls `StatusOr::StatusOr(const std::vector&)` + } + +In that case the fix is more consensual: just `return std::move(obj)`. +This is handled by the `-Wreturn-std-move` warning. diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp @@ -0,0 +1,92 @@ +// RUN: %check_clang_tidy -std=c++11,c++14,c++17,c++2a %s performance-no-automatic-move %t + +struct Obj { + Obj(); + Obj(const Obj &); + Obj(Obj &&); + virtual ~Obj(); +}; + +template +struct StatusOr { + StatusOr(const T &); + StatusOr(T &&); +}; + +struct NonTemplate { + NonTemplate(const Obj &); + NonTemplate(Obj &&); +}; + +template +T Make(); + +StatusOr PositiveStatusOrConstValue() { + const Obj obj = Make(); + return obj; + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents automatic move [performance-no-automatic-move] +} + +NonTemplate PositiveNonTemplateConstValue() { + const Obj obj = Make(); + return obj; + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents automatic move [performance-no-automatic-move] +} + +Obj PositiveSelfConstValue() { + const Obj obj = Make(); + return obj; + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents automatic move [performance-no-automatic-move] +} + +// FIXME: Ideally we would warn here too. +NonTemplate PositiveNonTemplateLifetimeExtension() { + const Obj &obj = Make(); + return obj; +} + +// FIXME: Ideally we would warn here too. +StatusOr PositiveStatusOrLifetimeExtension() { + const Obj &obj = Make(); + return obj; +} + +// Negatives. + +StatusOr Temporary() { + return Make(); +} + +StatusOr ConstTemporary() { + return Make(); +} + +StatusOr Nrvo() { + Obj obj = Make(); + return obj; +} + +StatusOr Ref() { + Obj &obj = Make(); + return obj; +} + +StatusOr ConstRef() { + const Obj &obj = Make(); + return obj; +} + +const Obj global; + +StatusOr Global() { + return global; +} + +struct FromConstRefOnly { + FromConstRefOnly(const Obj &); +}; + +FromConstRefOnly FromConstRefOnly() { + const Obj obj = Make(); + return obj; +}