[clang-tools-extra] 95fe549 - [clang-tidy] new performance-no-automatic-move check.
Clement Courbet via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 21 23:48:20 PST 2019
Author: Clement Courbet
Date: 2019-11-22T08:47:55+01:00
New Revision: 95fe54931fddccccf9740b3247219e30504da447
URL: https://github.com/llvm/llvm-project/commit/95fe54931fddccccf9740b3247219e30504da447
DIFF: https://github.com/llvm/llvm-project/commit/95fe54931fddccccf9740b3247219e30504da447.diff
LOG: [clang-tidy] new performance-no-automatic-move check.
Summary: The check flags constructs that prevent automatic move of local variables.
Reviewers: aaron.ballman
Subscribers: mgorny, xazax.hun, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D70390
Added:
clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.h
clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst
clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp
Modified:
clang-tools-extra/clang-tidy/performance/CMakeLists.txt
clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
index cde2e246bf9e..d1f9897b0154 100644
--- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
@@ -9,6 +9,7 @@ add_clang_library(clangTidyPerformanceModule
InefficientVectorOperationCheck.cpp
MoveConstArgCheck.cpp
MoveConstructorInitCheck.cpp
+ NoAutomaticMoveCheck.cpp
NoexceptMoveConstructorCheck.cpp
PerformanceTidyModule.cpp
TriviallyDestructibleCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
new file mode 100644
index 000000000000..d806c98d5c41
--- /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>("vardecl");
+ const auto *CtorCall = Result.Nodes.getNodeAs<Expr>("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/NoAutomaticMoveCheck.h b/clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.h
new file mode 100644
index 000000000000..ed521f1352e8
--- /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<std::string> 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/PerformanceTidyModule.cpp b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
index 269d09b98a68..87673a228da4 100644
--- 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 @@ class PerformanceModule : public ClangTidyModule {
"performance-move-const-arg");
CheckFactories.registerCheck<MoveConstructorInitCheck>(
"performance-move-constructor-init");
+ CheckFactories.registerCheck<NoAutomaticMoveCheck>(
+ "performance-no-automatic-move");
CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
"performance-noexcept-move-constructor");
CheckFactories.registerCheck<TriviallyDestructibleCheck>(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 09dc28ae1715..b96feecdf3d6 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -143,6 +143,11 @@ Improvements to clang-tidy
Finds Objective-C implementations that implement ``-isEqual:`` without also
appropriately implementing ``-hash``.
+- New :doc:`performance-no-automatic-move
+ <clang-tidy/checks/performance-no-automatic-move>` check.
+
+ Finds local variables that cannot be automatically moved due to constness.
+
- New :doc:`performance-trivially-destructible
<clang-tidy/checks/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
index 4a120684e3d3..a005ed8ef0a8 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -344,6 +344,7 @@ Clang-Tidy Checks
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
index 000000000000..a861238c5978
--- /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 <https://en.cppreference.com/w/cpp/language/return#automatic_move_from_local_variables_and_parameters>`_,
+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] <https://godbolt.org/z/x7SYYA>`_:
+
+.. code-block:: c++
+
+ StatusOr<std::vector<int>> Cool() {
+ std::vector<int> obj = ...;
+ return obj; // calls StatusOr::StatusOr(std::vector<int>&&)
+ }
+
+ StatusOr<std::vector<int>> NotCool() {
+ const std::vector<int> obj = ...;
+ return obj; // calls `StatusOr::StatusOr(const std::vector<int>&)`
+ }
+
+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<std::vector<int>> Uncool() {
+ std::vector<int>&& obj = ...;
+ return obj; // calls `StatusOr::StatusOr(const std::vector<int>&)`
+ }
+
+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
index 000000000000..40519a130580
--- /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 <typename T>
+struct StatusOr {
+ StatusOr(const T &);
+ StatusOr(T &&);
+};
+
+struct NonTemplate {
+ NonTemplate(const Obj &);
+ NonTemplate(Obj &&);
+};
+
+template <typename T>
+T Make();
+
+StatusOr<Obj> PositiveStatusOrConstValue() {
+ const Obj obj = Make<Obj>();
+ return obj;
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+NonTemplate PositiveNonTemplateConstValue() {
+ const Obj obj = Make<Obj>();
+ return obj;
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+Obj PositiveSelfConstValue() {
+ const Obj obj = Make<Obj>();
+ 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<Obj>();
+ return obj;
+}
+
+// FIXME: Ideally we would warn here too.
+StatusOr<Obj> PositiveStatusOrLifetimeExtension() {
+ const Obj &obj = Make<Obj>();
+ return obj;
+}
+
+// Negatives.
+
+StatusOr<Obj> Temporary() {
+ return Make<const Obj>();
+}
+
+StatusOr<Obj> ConstTemporary() {
+ return Make<const Obj>();
+}
+
+StatusOr<Obj> Nrvo() {
+ Obj obj = Make<Obj>();
+ return obj;
+}
+
+StatusOr<Obj> Ref() {
+ Obj &obj = Make<Obj &>();
+ return obj;
+}
+
+StatusOr<Obj> ConstRef() {
+ const Obj &obj = Make<Obj &>();
+ return obj;
+}
+
+const Obj global;
+
+StatusOr<Obj> Global() {
+ return global;
+}
+
+struct FromConstRefOnly {
+ FromConstRefOnly(const Obj &);
+};
+
+FromConstRefOnly FromConstRefOnly() {
+ const Obj obj = Make<Obj>();
+ return obj;
+}
More information about the cfe-commits
mailing list