[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-21 Thread Clement Courbet via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG95fe54931fdd: [clang-tidy] new performance-no-automatic-move 
check. (authored by courbet).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
  clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst
  clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp
===
--- /dev/null
+++ 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;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst
===
--- /dev/null
+++ 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.
Index: clang-tools-extra/docs/clang-tidy/check

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-21 Thread Clement Courbet via Phabricator via cfe-commits 
courbet marked an inline comment as done.
courbet added a comment.

Thanks for the review.




Comment at: clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp:32
+
+  const auto const_local_variable =
+  varDecl(hasLocalStorage(), unless(hasType(lValueReferenceType())),

JonasToth wrote:
> Last nits: all variables do not follow the LLVM convention of camel-casing. 
> Just realized now, sorry
Oh yes right, thanks for the catch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-21 Thread Clement Courbet via Phabricator via cfe-commits 
courbet updated this revision to Diff 230595.
courbet added a comment.

Use LLVM style


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
  clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst
  clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp
===
--- /dev/null
+++ 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;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst
===
--- /dev/null
+++ 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.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-ex

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-21 Thread Jonas Toth via Phabricator via cfe-commits 
JonasToth accepted this revision.
JonasToth added a comment.
This revision is now accepted and ready to land.

LGTM after the variable names are adjusted




Comment at: clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp:32
+
+  const auto const_local_variable =
+  varDecl(hasLocalStorage(), unless(hasType(lValueReferenceType())),

Last nits: all variables do not follow the LLVM convention of camel-casing. 
Just realized now, sorry


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-21 Thread Clement Courbet via Phabricator via cfe-commits 
courbet updated this revision to Diff 230428.
courbet marked 2 inline comments as done.
courbet added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
  clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst
  clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp
===
--- /dev/null
+++ 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;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst
===
--- /dev/null
+++ 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.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
=

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-20 Thread Jonas Toth via Phabricator via cfe-commits 
JonasToth added inline comments.



Comment at: clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp:28
+void NoAutomaticMoveCheck::registerMatchers(MatchFinder *finder) {
+  const auto const_local_variable =
+  varDecl(hasLocalStorage(), unless(hasType(lValueReferenceType())),

the matchers need only to be registered for c++ and and c++11 and later. See 
`getLangOpts()` in other checks.



Comment at: clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp:59
+  const QualType var_type = vardecl->getType();
+  if (var_type.isConstQualified()) {
+diag(ctor_call->getExprLoc(), "constness of '%0' prevents automatic move")

i think `isConstQualified` could move into the matcher with `Matcher 
isConstQualified`.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst:11
+local values are automatically moved out when returning from a function. A
+common mistake is to declared local ``lvalue`` variables ``const``, which
+prevents the move.

s/declared/declare


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-20 Thread Clement Courbet via Phabricator via cfe-commits 
courbet updated this revision to Diff 230202.
courbet added a comment.

Remove && warnings.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
  clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst
  clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp
===
--- /dev/null
+++ 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;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst
===
--- /dev/null
+++ 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 declared 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.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-to

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-20 Thread Clement Courbet via Phabricator via cfe-commits 
courbet added a comment.

In D70390#1751751 , @Quuxplusone wrote:

> that sounds like a big non-local change whose correctness would be very 
> difficult to verify.


Agreed. That's why I did not make this check suggest a fixit, because it's too 
easy to click "apply" without thinking.

>   I think (FWIW) that such a fixit would be a very hard sell

I've seen several places in our codebase where it did make quite a big 
difference performance wise, so they were quite easy to sell :)

> Nobody is currently working on that Clang patch AFAIK. If you're interested 
> in submitting Clang patches in this area, that might be a very high-value (if 
> kind of fuzzy/user-experience/political) patch to work on!
>  Meanwhile, as of November 2019, the C++2a committee draft still contains 
> unfixed corner cases that Clang doesn't warn about. For example:
>  https://godbolt.org/z/fBKM-4 (no warning from Clang; C++2a fixes this 
> behavior)
>  https://godbolt.org/z/EfBCdQ (no warning from Clang; C++2a does NOT fix this 
> behavior)

Super interesting, thanks ! I'll see if I have time to work on that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-20 Thread Clement Courbet via Phabricator via cfe-commits 
courbet planned changes to this revision.
courbet added a comment.

I'm going to remove the `&&` warning from the check and keep only the `const` 
one, as I think these is a consensus on that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-19 Thread Arthur O'Dwyer via Phabricator via cfe-commits 
Quuxplusone added a comment.

Hi, I'm the main original author (although I would not claim "current 
maintainer") of the Clang warning.
Clang doesn't warn about `const vector v; return v;` because in that case, 
adding `std::move` to the return would not help anything. I had not considered 
that we might suggest a "fixit" of "Change the declared type of this variable 
from `vector` to `const vector`"; that sounds like a big non-local change whose 
correctness would be very difficult to verify. I think (FWIW) that such a fixit 
would be a very hard sell, but at the same time I admit that I had not even 
thought of it when I was working on the diagnostic originally.

If you're working in this area, I would also caution you that C++2a is going to 
completely rewrite the rules in this area. In C++2a, everything that Clang 
currently warns about is going to be //fixed in the standard//, so that the 
Clang warning becomes merely part of `-Wc++17-compat`. Nobody is currently 
working on that Clang patch AFAIK. If you're interested in submitting Clang 
patches in this area, that might be a very high-value (if kind of 
fuzzy/user-experience/political) patch to work on!

Meanwhile, as of November 2019, the C++2a committee draft still contains 
unfixed corner cases that Clang doesn't warn about. For example:
https://godbolt.org/z/fBKM-4 (no warning from Clang; C++2a fixes this behavior)
https://godbolt.org/z/EfBCdQ (no warning from Clang; C++2a does NOT fix this 
behavior)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-19 Thread Jonas Toth via Phabricator via cfe-commits 
JonasToth added a comment.

In D70390#1751159 , @courbet wrote:

> > IMHO these two should just not overlap. It makes sense, to have 
> > controversial or configurable stuff in clang-tidy. It should just be 
> > consistent with the warnings, as those are "always right" and clang-tidy 
> > can be opinionated/specialized.
>
> So to make sure I understand you're advocating for keeping the `const` 
> version in the clang-tidy check but removing the `&&` detection from this 
> check and let the warning deal with that ?


Yes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-19 Thread Clement Courbet via Phabricator via cfe-commits 
courbet added a comment.

> IMHO these two should just not overlap. It makes sense, to have controversial 
> or configurable stuff in clang-tidy. It should just be consistent with the 
> warnings, as those are "always right" and clang-tidy can be 
> opinionated/specialized.

So to make sure I understand you're advocating for keeping the `const` version 
in the clang-tidy check but removing the `&&` detection from this check and let 
the warning deal with that ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-19 Thread Jonas Toth via Phabricator via cfe-commits 
JonasToth added a comment.

IMHO these two should just not overlap. It makes sense, to have controversial 
or configurable stuff in clang-tidy. It should just be consistent with the 
warnings, as those are "always right" and clang-tidy can be 
opinionated/specialized.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-19 Thread Clement Courbet via Phabricator via cfe-commits 
courbet added a comment.

Adding @Quuxplusone (warning author) for opinions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-19 Thread Clement Courbet via Phabricator via cfe-commits 
courbet reclaimed this revision.
courbet added a comment.

Actually, thinking more about this, adding this to the existing warning might 
not be a very consensual change. In the case of the warning:

  S f() {
T&& t = ...;
...
return t;
  }

there is no argument against doing the std::move(): the code is just as clear 
and has better performance:

  S f() {
T&& t = ...;
...
return std::move(t);
  }

In the case of a const variable, some people might value the safety from the 
const above the performance:

  S f() {
const T t = ...;
...  // here be dragons
return t;
  }

And they would be unhappy if the -Wreturn-std-move started warning about this.

So I think there are two options here:

- Add a new warning, or
- Add it as a clang-tidy as in this change.

What do you think ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-18 Thread Clement Courbet via Phabricator via cfe-commits 
courbet abandoned this revision.
courbet marked an inline comment as done.
courbet added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst:47
+std::vector&& obj = ...;
+return std::move(obj);  // calls StatusOr::StatusOr(std::vector&&)
+  }

lebedev.ri wrote:
> JonasToth wrote:
> > lebedev.ri wrote:
> > > courbet wrote:
> > > > JonasToth wrote:
> > > > > While checking this example it seems clang already has a warning for 
> > > > > that case?
> > > > > 
> > > > > https://godbolt.org/z/q5zzh-
> > > > > 
> > > > > What parts of this check will be more then the warnings already do?
> > > > I was not aware of that, thanks for pointing that out. I don't think 
> > > > the check does more than the warning in that case. TBH I have not seen 
> > > > instances of this while running the check on our codebase (I'm only 
> > > > looking at a sample of the mistakes though, there are too many hits to 
> > > > look at all of them). All mistakes I have seen are of the `const` kind. 
> > > > 
> > > > The value we get from having this in the form of a check is more 
> > > > control over which types are allowed through the clang-tidy options.
> > > The `const std::vector f` isn't diagnosed by the existing diag: 
> > > https://godbolt.org/z/ZTQ3H6
> > so should we split this up in a diagnostic and a clang-tidy check? maybe it 
> > makes more sense to improve the warning further?
> I'd +1 just enhancing the clang diagnostic.
> The downside is that it does not allow blacklist, sure.
OK I'll work on improving the warning.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-18 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst:47
+std::vector&& obj = ...;
+return std::move(obj);  // calls StatusOr::StatusOr(std::vector&&)
+  }

JonasToth wrote:
> lebedev.ri wrote:
> > courbet wrote:
> > > JonasToth wrote:
> > > > While checking this example it seems clang already has a warning for 
> > > > that case?
> > > > 
> > > > https://godbolt.org/z/q5zzh-
> > > > 
> > > > What parts of this check will be more then the warnings already do?
> > > I was not aware of that, thanks for pointing that out. I don't think the 
> > > check does more than the warning in that case. TBH I have not seen 
> > > instances of this while running the check on our codebase (I'm only 
> > > looking at a sample of the mistakes though, there are too many hits to 
> > > look at all of them). All mistakes I have seen are of the `const` kind. 
> > > 
> > > The value we get from having this in the form of a check is more control 
> > > over which types are allowed through the clang-tidy options.
> > The `const std::vector f` isn't diagnosed by the existing diag: 
> > https://godbolt.org/z/ZTQ3H6
> so should we split this up in a diagnostic and a clang-tidy check? maybe it 
> makes more sense to improve the warning further?
I'd +1 just enhancing the clang diagnostic.
The downside is that it does not allow blacklist, sure.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-18 Thread Jonas Toth via Phabricator via cfe-commits 
JonasToth added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst:47
+std::vector&& obj = ...;
+return std::move(obj);  // calls StatusOr::StatusOr(std::vector&&)
+  }

lebedev.ri wrote:
> courbet wrote:
> > JonasToth wrote:
> > > While checking this example it seems clang already has a warning for that 
> > > case?
> > > 
> > > https://godbolt.org/z/q5zzh-
> > > 
> > > What parts of this check will be more then the warnings already do?
> > I was not aware of that, thanks for pointing that out. I don't think the 
> > check does more than the warning in that case. TBH I have not seen 
> > instances of this while running the check on our codebase (I'm only looking 
> > at a sample of the mistakes though, there are too many hits to look at all 
> > of them). All mistakes I have seen are of the `const` kind. 
> > 
> > The value we get from having this in the form of a check is more control 
> > over which types are allowed through the clang-tidy options.
> The `const std::vector f` isn't diagnosed by the existing diag: 
> https://godbolt.org/z/ZTQ3H6
so should we split this up in a diagnostic and a clang-tidy check? maybe it 
makes more sense to improve the warning further?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-18 Thread Clement Courbet via Phabricator via cfe-commits 
courbet updated this revision to Diff 229840.
courbet added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
  clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst
  clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp
@@ -0,0 +1,165 @@
+// RUN: %check_clang_tidy -std=c++11,c++14,c++17 %s performance-no-automatic-move %t
+
+namespace std {
+template 
+struct remove_reference;
+
+template 
+struct remove_reference {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &> {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &&> {
+  typedef _Tp type;
+};
+
+template 
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) {
+  return static_cast::type &&>(__t);
+}
+
+template 
+constexpr _Tp &&
+forward(typename remove_reference<_Tp>::type &__t) noexcept {
+  return static_cast<_Tp &&>(__t);
+}
+
+} // namespace std
+
+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]
+}
+
+StatusOr PositiveStatusOrRefRef() {
+  Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+StatusOr PositiveNonTemplateRefRef() {
+  Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+StatusOr PositiveSelfRefRef() {
+  Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+StatusOr PositiveStatusOrConstRefRef() {
+  const Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+NonTemplate PositiveNonTemplateConstRefRef() {
+  const Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+NonTemplate PositiveSelfConstRefRef() {
+  const Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness 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;
+}
+
+NonTemplate RefRefMove() {
+  Obj &&obj = Make();
+  return std::move(obj);
+}
+
+const Obj global;
+
+StatusOr Global() {
+  return global;
+}
+
+struct FromConstRefOnly {
+  FromConstRefOnly(const Obj &);
+};
+
+FromConstRefOnly FromConstRefOnly() {
+  const Obj obj = Make();
+  return obj;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst
@@ -0,0 +1,61 @@
+.. title:: clang-tidy - performa

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-18 Thread Clement Courbet via Phabricator via cfe-commits 
courbet marked 2 inline comments as done.
courbet added a comment.

Thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-18 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst:47
+std::vector&& obj = ...;
+return std::move(obj);  // calls StatusOr::StatusOr(std::vector&&)
+  }

courbet wrote:
> JonasToth wrote:
> > While checking this example it seems clang already has a warning for that 
> > case?
> > 
> > https://godbolt.org/z/q5zzh-
> > 
> > What parts of this check will be more then the warnings already do?
> I was not aware of that, thanks for pointing that out. I don't think the 
> check does more than the warning in that case. TBH I have not seen instances 
> of this while running the check on our codebase (I'm only looking at a sample 
> of the mistakes though, there are too many hits to look at all of them). All 
> mistakes I have seen are of the `const` kind. 
> 
> The value we get from having this in the form of a check is more control over 
> which types are allowed through the clang-tidy options.
The `const std::vector f` isn't diagnosed by the existing diag: 
https://godbolt.org/z/ZTQ3H6


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-18 Thread Eugene Zelenko via Phabricator via cfe-commits 
Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:143
+
+  The check flags constructs that prevent automatic move of local variables.
+

Please omit //The check// and synchronize with first sentence of documentation.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst:10
+
+This check detects some usage patterns that prevent this optimization from
+happening. The most common one is local ``lvalue`` variables that are declared

Please omit //This check//,


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-18 Thread Clement Courbet via Phabricator via cfe-commits 
courbet marked 3 inline comments as done.
courbet added a comment.

Thanks for the comments




Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst:47
+std::vector&& obj = ...;
+return std::move(obj);  // calls StatusOr::StatusOr(std::vector&&)
+  }

JonasToth wrote:
> While checking this example it seems clang already has a warning for that 
> case?
> 
> https://godbolt.org/z/q5zzh-
> 
> What parts of this check will be more then the warnings already do?
I was not aware of that, thanks for pointing that out. I don't think the check 
does more than the warning in that case. TBH I have not seen instances of this 
while running the check on our codebase (I'm only looking at a sample of the 
mistakes though, there are too many hits to look at all of them). All mistakes 
I have seen are of the `const` kind. 

The value we get from having this in the form of a check is more control over 
which types are allowed through the clang-tidy options.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst:55
+
+## Semantics
+

JonasToth wrote:
> Does this syntax work in rst files?
Nope, this was migrated from our internal markdown syntax, thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-18 Thread Clement Courbet via Phabricator via cfe-commits 
courbet updated this revision to Diff 229833.
courbet marked an inline comment as done.
courbet added a comment.

Fix markdown in doc.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
  clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst
  clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp
@@ -0,0 +1,165 @@
+// RUN: %check_clang_tidy -std=c++11,c++14,c++17 %s performance-no-automatic-move %t
+
+namespace std {
+template 
+struct remove_reference;
+
+template 
+struct remove_reference {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &> {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &&> {
+  typedef _Tp type;
+};
+
+template 
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) {
+  return static_cast::type &&>(__t);
+}
+
+template 
+constexpr _Tp &&
+forward(typename remove_reference<_Tp>::type &__t) noexcept {
+  return static_cast<_Tp &&>(__t);
+}
+
+} // namespace std
+
+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]
+}
+
+StatusOr PositiveStatusOrRefRef() {
+  Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+StatusOr PositiveNonTemplateRefRef() {
+  Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+StatusOr PositiveSelfRefRef() {
+  Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+StatusOr PositiveStatusOrConstRefRef() {
+  const Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+NonTemplate PositiveNonTemplateConstRefRef() {
+  const Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+NonTemplate PositiveSelfConstRefRef() {
+  const Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness 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;
+}
+
+NonTemplate RefRefMove() {
+  Obj &&obj = Make();
+  return std::move(obj);
+}
+
+const Obj global;
+
+StatusOr Global() {
+  return global;
+}
+
+struct FromConstRefOnly {
+  FromConstRefOnly(const Obj &);
+};
+
+FromConstRefOnly FromConstRefOnly() {
+  const Obj obj = Make();
+  return obj;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst
@@ -0

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-18 Thread Jonas Toth via Phabricator via cfe-commits 
JonasToth added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst:47
+std::vector&& obj = ...;
+return std::move(obj);  // calls StatusOr::StatusOr(std::vector&&)
+  }

While checking this example it seems clang already has a warning for that case?

https://godbolt.org/z/q5zzh-

What parts of this check will be more then the warnings already do?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst:55
+
+## Semantics
+

Does this syntax work in rst files?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70390/new/

https://reviews.llvm.org/D70390



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

2019-11-18 Thread Clement Courbet via Phabricator via cfe-commits 
courbet created this revision.
courbet added a reviewer: aaron.ballman.
Herald added subscribers: xazax.hun, mgorny.
Herald added a project: clang.

The check flags constructs that prevent automatic move of local variables.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70390

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
  clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst
  clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp
@@ -0,0 +1,165 @@
+// RUN: %check_clang_tidy -std=c++11,c++14,c++17 %s performance-no-automatic-move %t
+
+namespace std {
+template 
+struct remove_reference;
+
+template 
+struct remove_reference {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &> {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &&> {
+  typedef _Tp type;
+};
+
+template 
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) {
+  return static_cast::type &&>(__t);
+}
+
+template 
+constexpr _Tp &&
+forward(typename remove_reference<_Tp>::type &__t) noexcept {
+  return static_cast<_Tp &&>(__t);
+}
+
+} // namespace std
+
+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]
+}
+
+StatusOr PositiveStatusOrRefRef() {
+  Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+StatusOr PositiveNonTemplateRefRef() {
+  Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+StatusOr PositiveSelfRefRef() {
+  Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+StatusOr PositiveStatusOrConstRefRef() {
+  const Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+NonTemplate PositiveNonTemplateConstRefRef() {
+  const Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+NonTemplate PositiveSelfConstRefRef() {
+  const Obj &&obj = Make();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: rvalue-ness 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;
+}
+
+NonTemplate RefRefMove() {
+  Obj &&obj = Make();
+  return std::move(obj);
+}
+
+const Obj global;
+
+StatusOr Global() {
+  return global;
+}
+
+struct FromConstRefOnly {
+  FromConstRefOnly(const Obj &);
+};
+
+FromConstRefOnly FromConstRefOnly() {
+  const Obj obj = Make();
+  return obj;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance-no-a