[PATCH] D107900: Add a new clang-tidy check for trivially copyable types passed by const reference

[Julien Marrec via Phabricator via cfe-commits]

jmarrec created this revision.
jmarrec added a reviewer: steveire.
Herald added a subscriber: mgorny.
jmarrec requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107900

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.cpp
  clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp
@@ -0,0 +1,55 @@
+// RUN: %check_clang_tidy %s misc-pod-const-ref-to-value %t
+
+int f1(const int &i);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f1(int i);
+int f2(const int &i, double d);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f2(int i, double d);
+
+int f3(double d, const int &i);
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f3(double d, int i);
+
+int f4(const double &d, const double &d2);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-MESSAGES: :[[@LINE-2]]:25: warning: argument 'd2' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f4(double d, double d2);
+
+namespace n1 {
+class A {
+  static int g(const double &d);
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+  // CHECK-FIXES: static int g(double d);
+};
+
+int A::g(const double &d) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+  // CHECK-FIXES: int A::g(double d) {
+  return static_cast(d);
+}
+} // namespace n1
+
+// Not triggering the check
+
+int f5(int i);
+int f6(int i, double d);
+int f7(int &i); // Might be used for return...
+
+namespace n2 {
+class A {
+  static int g(double d);
+};
+
+int A::g(double d) {
+  return static_cast(d);
+}
+
+class B {
+  static int g(double &d);
+};
+
+int B::g(double &d) {
+  return static_cast(d);
+}
+} // namespace n2
Index: clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
@@ -0,0 +1,6 @@
+.. title:: clang-tidy - misc-pod-const-ref-to-value
+
+misc-pod-const-ref-to-value
+===
+
+FIXME: Describe what patterns does the check detect and why. Give examples.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -212,6 +212,7 @@
`misc-no-recursion `_,
`misc-non-copyable-objects `_,
`misc-non-private-member-variables-in-classes `_,
+   `misc-pod-const-ref-to-value `_, "Yes"
`misc-redundant-expression `_, "Yes"
`misc-static-assert `_, "Yes"
`misc-throw-by-value-catch-by-reference `_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -72,6 +72,12 @@
 New checks
 ^^
 
+- New :doc:`misc-pod-const-ref-to-value
+  ` check.
+
+  Finds trivially_copyable types are passed by const-reference to a function
+  and changes that to by value
+
 New check aliases
 ^
 
Index: clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.h
@@ -0,0 +1,41 @@
+//===--- PodConstRefToValueCheck.h - clang-tidy -*- C++ -*-===//
+//
+// Part of the LLVM Proje

[PATCH] D107900: Add a new clang-tidy check for trivially copyable types passed by const reference

[Julien Marrec via Phabricator via cfe-commits]

jmarrec updated this revision to Diff 365735.
jmarrec added a comment.

Added content to the 
`clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst` file


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

https://reviews.llvm.org/D107900

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.cpp
  clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp
@@ -0,0 +1,55 @@
+// RUN: %check_clang_tidy %s misc-pod-const-ref-to-value %t
+
+int f1(const int &i);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f1(int i);
+int f2(const int &i, double d);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f2(int i, double d);
+
+int f3(double d, const int &i);
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f3(double d, int i);
+
+int f4(const double &d, const double &d2);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-MESSAGES: :[[@LINE-2]]:25: warning: argument 'd2' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f4(double d, double d2);
+
+namespace n1 {
+class A {
+  static int g(const double &d);
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+  // CHECK-FIXES: static int g(double d);
+};
+
+int A::g(const double &d) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+  // CHECK-FIXES: int A::g(double d) {
+  return static_cast(d);
+}
+} // namespace n1
+
+// Not triggering the check
+
+int f5(int i);
+int f6(int i, double d);
+int f7(int &i); // Might be used for return...
+
+namespace n2 {
+class A {
+  static int g(double d);
+};
+
+int A::g(double d) {
+  return static_cast(d);
+}
+
+class B {
+  static int g(double &d);
+};
+
+int B::g(double &d) {
+  return static_cast(d);
+}
+} // namespace n2
Index: clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
@@ -0,0 +1,21 @@
+.. title:: clang-tidy - misc-pod-const-ref-to-value
+
+misc-pod-const-ref-to-value
+===
+
+This check detects when ``trivially_copyable`` types are passed by const-reference
+to a function and changes that to by value
+
+For example in the following code, it is replaced by ``void f(int i, double d)``
+
+.. code-block:: c++
+
+  void f(const int& i, const double& d);
+
+
+Options
+---
+
+.. option:: RestrictToBuiltInTypes
+
+   If set to `true`, this check will limit itself to the `builtinType()` types. Default is `false`.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -212,6 +212,7 @@
`misc-no-recursion `_,
`misc-non-copyable-objects `_,
`misc-non-private-member-variables-in-classes `_,
+   `misc-pod-const-ref-to-value `_, "Yes"
`misc-redundant-expression `_, "Yes"
`misc-static-assert `_, "Yes"
`misc-throw-by-value-catch-by-reference `_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -72,6 +72,12 @@
 New checks
 ^^
 
+- New :doc:`misc-pod-const-ref-to-value
+  ` check.
+
+  Finds trivially_copyable types are passed by const-reference to a function
+  and changes that to by value
+
 New check aliases
 ^
 
I

[PATCH] D107900: Add a new clang-tidy check for trivially copyable types passed by const reference

[Julien Marrec via Phabricator via cfe-commits]

jmarrec added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp:3-5
+int f1(const int &i);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'i' is a trivially 
copyable type and should not be passed by const-reference but by value 
[misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f1(int i);

This is now failing after I applied clang-format.  `int f1(const int& i)` would 
correctly produce `int f1(int i);`, now it does `int f1(inti);`... I need to 
find a way to fix that.


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

https://reviews.llvm.org/D107900

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

[PATCH] D107900: Add a new clang-tidy check for trivially copyable types passed by const reference

[Julien Marrec via Phabricator via cfe-commits]

jmarrec added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst:21
+
+   If set to `true`, this check will limit itself to the `builtinType()` 
types. Default is `false`.

Quuxplusone wrote:
> D107873 is related.
> 
> I'd like to see some tests/discussion around large types, e.g.
> ```
> struct Widget { int a[1000]; };
> void f(const Widget&);
> ```
> (I think D107873 at least makes a conscious attempt to get the "would it be 
> passed in registers?" check right.)
> 
> I'd like to see some tests/discussion around generic code, e.g.
> ```
> template
> struct Max {
> static T max(const T& a, const T& b);
> };
> int x = Max::max(1, 2);  // passes `int` by const reference, but this is 
> fine
> ```
Should I close this one in favor of https://reviews.llvm.org/D107873?


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

https://reviews.llvm.org/D107900

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

[PATCH] D107873: [WIP][clang-tidy] adds a const-qualified parameter check

[Julien Marrec via Phabricator via cfe-commits]

jmarrec added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst:90
+Passing a ``shared_ptr`` by reference-to-``const`` is also acceptable in 
certain
+situaitons. As such, ``std::shared_ptr`` and ``boost::shared_ptr`` will
+not warn in either case by default.





Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst:101
+
+The default value for ``SmallMaxSize`` was determined by through benchmarking
+when passing by value was no longer competetive with passing by reference for





Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst:104
+various `boards `_. The benchmark can be found on
+`Compiler Explorer `_, with the used to inform
+the threshold.

typo here, not sure what the intent was.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107873

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

[PATCH] D107900: Add a new clang-tidy check for trivially copyable types passed by const reference

[Julien Marrec via Phabricator via cfe-commits]

jmarrec updated this revision to Diff 365778.
jmarrec added a comment.

Added a MaxSize option. defaults to 16. Also skip templates. like in D107873 


Dealt with the `f(int &i)` case instead of `f(int& i)`


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

https://reviews.llvm.org/D107900

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.cpp
  clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp
@@ -0,0 +1,73 @@
+// RUN: %check_clang_tidy %s misc-pod-const-ref-to-value %t
+
+int f1(const int &i);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f1(int i);
+int f2(const int &i, double d);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f2(int i, double d);
+
+int f3(double d, const int &i);
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f3(double d, int i);
+
+int f4(const double &d, const double &d2);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-MESSAGES: :[[@LINE-2]]:25: warning: argument 'd2' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f4(double d, double d2);
+
+// clang-format off
+int f5(const int& i);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f5(int i);
+// clang-format on
+
+namespace n1 {
+class A {
+  static int g(const double &d);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+  // CHECK-FIXES: static int g(double d);
+};
+
+int A::g(const double &d) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+  // CHECK-FIXES: int A::g(double d) {
+  return static_cast(d);
+}
+} // namespace n1
+
+// Not triggering the check
+
+int f5(int i);
+int f6(int i, double d);
+int f7(int &i); // Might be used for return...
+
+namespace n2 {
+class A {
+  static int g(double d);
+};
+
+int A::g(double d) {
+  return static_cast(d);
+}
+
+class B {
+  static int g(double &d);
+};
+
+int B::g(double &d) {
+  return static_cast(d);
+}
+} // namespace n2
+
+template 
+void f(Args&&... args);
+
+struct Widget { int a[1000]; };
+void f(const Widget&);
+
+template
+struct Max {
+static T max(const T& a, const T& b);
+};
+int x = Max::max(1, 2);  // passes `int` by const reference, but this is fine
Index: clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
@@ -0,0 +1,25 @@
+.. title:: clang-tidy - misc-pod-const-ref-to-value
+
+misc-pod-const-ref-to-value
+===
+
+This check detects when ``trivially_copyable`` types are passed by const-reference
+to a function and changes that to by value
+
+For example in the following code, it is replaced by ``void f(int i, double d)``
+
+.. code-block:: c++
+
+  void f(const int& i, const double& d);
+
+
+Options
+---
+
+.. option:: RestrictToBuiltInTypes
+
+   If set to `true`, this check will limit itself to the `builtinType()` types. Default is `false`.
+
+.. option:: MaxSize
+
+   MaxSize: int, default 16. Above this size, passing by const-reference makes sense
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -212,6 +212,7 @@
`misc-no-recursion `_,
`misc-no

[PATCH] D107900: Add a new clang-tidy check for trivially copyable types passed by const reference

[Julien Marrec via Phabricator via cfe-commits]

jmarrec updated this revision to Diff 365779.
jmarrec added a comment.

Forgot to run clang format on the changes


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

https://reviews.llvm.org/D107900

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.cpp
  clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp
@@ -0,0 +1,75 @@
+// RUN: %check_clang_tidy %s misc-pod-const-ref-to-value %t
+
+int f1(const int &i);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f1(int i);
+int f2(const int &i, double d);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f2(int i, double d);
+
+int f3(double d, const int &i);
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f3(double d, int i);
+
+int f4(const double &d, const double &d2);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-MESSAGES: :[[@LINE-2]]:25: warning: argument 'd2' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f4(double d, double d2);
+
+// clang-format off
+int f5(const int& i);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f5(int i);
+// clang-format on
+
+namespace n1 {
+class A {
+  static int g(const double &d);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+  // CHECK-FIXES: static int g(double d);
+};
+
+int A::g(const double &d) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+  // CHECK-FIXES: int A::g(double d) {
+  return static_cast(d);
+}
+} // namespace n1
+
+// Not triggering the check
+
+int f5(int i);
+int f6(int i, double d);
+int f7(int &i); // Might be used for return...
+
+namespace n2 {
+class A {
+  static int g(double d);
+};
+
+int A::g(double d) {
+  return static_cast(d);
+}
+
+class B {
+  static int g(double &d);
+};
+
+int B::g(double &d) {
+  return static_cast(d);
+}
+} // namespace n2
+
+template 
+void f(Args &&...args);
+
+struct Widget {
+  int a[1000];
+};
+void f(const Widget &);
+
+template 
+struct Max {
+  static T max(const T &a, const T &b);
+};
+int x = Max::max(1, 2); // passes `int` by const reference, but this is fine
Index: clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
@@ -0,0 +1,25 @@
+.. title:: clang-tidy - misc-pod-const-ref-to-value
+
+misc-pod-const-ref-to-value
+===
+
+This check detects when ``trivially_copyable`` types are passed by const-reference
+to a function and changes that to by value
+
+For example in the following code, it is replaced by ``void f(int i, double d)``
+
+.. code-block:: c++
+
+  void f(const int& i, const double& d);
+
+
+Options
+---
+
+.. option:: RestrictToBuiltInTypes
+
+   If set to `true`, this check will limit itself to the `builtinType()` types. Default is `false`.
+
+.. option:: MaxSize
+
+   MaxSize: int, default 16. Above this size, passing by const-reference makes sense
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -212,6 +212,7 @@
`misc-no-recursion `_,
`misc-non-copyable-objects `_,
`misc-non-private-member-variables-in-classes `_,
+   `misc-pod-const-ref-to-value `_, "Yes"

[PATCH] D107900: Add a new clang-tidy check for trivially copyable types passed by const reference

[Julien Marrec via Phabricator via cfe-commits]

jmarrec added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst:21
+
+   If set to `true`, this check will limit itself to the `builtinType()` 
types. Default is `false`.

cjdb wrote:
> jmarrec wrote:
> > Quuxplusone wrote:
> > > D107873 is related.
> > > 
> > > I'd like to see some tests/discussion around large types, e.g.
> > > ```
> > > struct Widget { int a[1000]; };
> > > void f(const Widget&);
> > > ```
> > > (I think D107873 at least makes a conscious attempt to get the "would it 
> > > be passed in registers?" check right.)
> > > 
> > > I'd like to see some tests/discussion around generic code, e.g.
> > > ```
> > > template
> > > struct Max {
> > > static T max(const T& a, const T& b);
> > > };
> > > int x = Max::max(1, 2);  // passes `int` by const reference, but 
> > > this is fine
> > > ```
> > Should I close this one in favor of https://reviews.llvm.org/D107873?
> Up to you. There's always a chance that D107873 doesn't get approved. What I 
> find most interesting is that we were independently working on this at the 
> same time :-)
@cjdb I guess it's uncanny that we would both decide to do it at the same time. 
A bit unfortunate too probably, I could have saved a few hours :) but I learned 
something about clang-tidy so I don't mind at all!

Yours (D107873) is definitely better documented, you handle to opposite case (I 
only deal with const-ref -> value, not value -> const ret) and I just borrowed 
your implementation for MaxSize / ignoring shared_ptr

As far as I can tell from a cursory look, there are a couple of things I'm 
doing that you aren't:

* I am actually using Options, I don't think you are yet, even for MaxSize. I 
see from your documentation file that you plan to add a few
* The RestrictToBuiltInTypes I find an interesting option too
* I am removing the const as well in addition to the `&`. I guess you are just 
relying on using it in conjunction with 
`readability-avoid-const-params-in-decls` but I think that makes it harder to 
use (you can't just do -checks=-*, you have to remember to add the 
readability-avoid-const-params-in-decls too...).

Perhaps we could discuss adding the above to your PR and consolidate over there 
to make a nice, feature-complete check?


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

https://reviews.llvm.org/D107900

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

[PATCH] D107873: [clang-tidy] Add 'performance-const-parameter-value-or-ref' for checking const-qualified parameters

[Julien Marrec via Phabricator via cfe-commits]

jmarrec added a comment.

Imported some discussion from https://reviews.llvm.org/D107900#inline-1029358. 
Sorry it took that long




Comment at: 
clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.h:40
+  static constexpr int DefaultSmallMaxSize = 16;
+  int SmallMaxSize = Options.get("SmallMaxSize", DefaultSmallMaxSize);
+

https://reviews.llvm.org/D107900#inline-1029358


On mine, I had another option `RestrictToBuiltInTypes` which I find an 
interesting option. It has a ` builtinType()` matcher in the registerMatchers 
function



Comment at: 
clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.h:43
+  bool ForwardDeclarationsSuppressWarnings =
+  Options.get("ForwardDeclarationsSuppressWarnings", true);
+

Perhaps two additional options to turn off either "side" would be helpful? eg 
`CheckPassByValueIsAppropriate` and `CheckPassByRefIsAppropriate`, both 
defaulted to `true`



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst:44
+  // warning: 'const int' is a small, trivially copyable type: pass by value 
instead
+  // fix-it: void f6(const int i) {}
+

https://reviews.llvm.org/D107900#inline-1029358

I am removing the `const` as well in addition to the `&`. I guess you may be 
just relying on using it in conjunction with 
`readability-avoid-const-params-in-decls` but I think that makes it harder to 
use (you can't just do `-checks=-*,` you have to remember to add the 
`readability-avoid-const-params-in-decls` too...).

Taht being said, perhaps that's how it's meant to be...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107873

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

[PATCH] D107900: [clang-tidy] Add a new clang-tidy check for trivially copyable types passed by const reference

[Julien Marrec via Phabricator via cfe-commits]

jmarrec updated this revision to Diff 368881.
jmarrec added a comment.

documentation updates


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107900

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.cpp
  clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp
@@ -0,0 +1,75 @@
+// RUN: %check_clang_tidy %s misc-pod-const-ref-to-value %t
+
+int f1(const int &i);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f1(int i);
+int f2(const int &i, double d);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f2(int i, double d);
+
+int f3(double d, const int &i);
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f3(double d, int i);
+
+int f4(const double &d, const double &d2);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-MESSAGES: :[[@LINE-2]]:25: warning: argument 'd2' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f4(double d, double d2);
+
+// clang-format off
+int f5(const int& i);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f5(int i);
+// clang-format on
+
+namespace n1 {
+class A {
+  static int g(const double &d);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+  // CHECK-FIXES: static int g(double d);
+};
+
+int A::g(const double &d) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+  // CHECK-FIXES: int A::g(double d) {
+  return static_cast(d);
+}
+} // namespace n1
+
+// Not triggering the check
+
+int f5(int i);
+int f6(int i, double d);
+int f7(int &i); // Might be used for return...
+
+namespace n2 {
+class A {
+  static int g(double d);
+};
+
+int A::g(double d) {
+  return static_cast(d);
+}
+
+class B {
+  static int g(double &d);
+};
+
+int B::g(double &d) {
+  return static_cast(d);
+}
+} // namespace n2
+
+template 
+void f(Args &&...args);
+
+struct Widget {
+  int a[1000];
+};
+void f(const Widget &);
+
+template 
+struct Max {
+  static T max(const T &a, const T &b);
+};
+int x = Max::max(1, 2); // passes `int` by const reference, but this is fine
Index: clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
@@ -0,0 +1,25 @@
+.. title:: clang-tidy - misc-pod-const-ref-to-value
+
+misc-pod-const-ref-to-value
+===
+
+This check detects when ``trivially_copyable`` types are passed by const-reference
+to a function and changes that to by value/
+
+For example in the following code, it is replaced by ``void f(int i, double d)``:
+
+.. code-block:: c++
+
+  void f(const int& i, const double& d);
+
+
+Options
+---
+
+.. option:: RestrictToBuiltInTypes
+
+   If set to `true`, this check will limit itself to the `builtinType()` types. Default is `false`.
+
+.. option:: MaxSize
+
+   MaxSize: int, default 16. Above this size, passing by const-reference makes sense
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -215,6 +215,7 @@
`misc-no-recursion `_,
`misc-non-copyable-objects `_,
`misc-non-private-member-variables-in-classes `_,
+   `misc-pod-const-ref-to