fix(clang-tidy): performance fixes applied in tests and CI (#3051)

* Initial fixes

* Whoops

* Finish clang-tidy manual fixes

* Add two missing fixes

* Revert

* Update clang-tidy

* Try to fix unreachable code error

* Move nolint comment

* Apply missing fix

* Don't override clang-tidy config

* Does this fix clang-tidy?

* Make all clang-tidy errors visible

* Add comments about NOLINTs and remove a few

* Fix typo

Author: Skylion007

.clang-tidy

@@ -18,6 +18,7 @@ modernize-use-noexcept,
 modernize-use-emplace,
 modernize-use-override,
 modernize-use-using,
+*performance*,
 readability-container-size-empty,
 readability-make-member-function-const,
 readability-redundant-function-ptr-dereference,

.github/workflows/format.yml

@@ -27,7 +27,7 @@ jobs:
   clang-tidy:
     name: Clang-Tidy
     runs-on: ubuntu-latest
-    container: silkeh/clang:10
+    container: silkeh/clang:12
     steps:
     - uses: actions/checkout@v2
 
@@ -37,10 +37,10 @@ jobs:
     - name: Configure
       run: >
         cmake -S . -B build
-        -DCMAKE_CXX_CLANG_TIDY="$(which clang-tidy);--warnings-as-errors=*"
+        -DCMAKE_CXX_CLANG_TIDY="$(which clang-tidy)"
         -DDOWNLOAD_EIGEN=ON
         -DDOWNLOAD_CATCH=ON
         -DCMAKE_CXX_STANDARD=17
 
     - name: Build
-      run: cmake --build build -j 2
+      run: cmake --build build -j 2 -- --keep-going

include/pybind11/iostream.h

@@ -262,7 +262,7 @@ add_ostream_redirect(module_ m, const std::string &name = "ostream_redirect") {
     return class_<detail::OstreamRedirect>(std::move(m), name.c_str(), module_local())
         .def(init<bool, bool>(), arg("stdout") = true, arg("stderr") = true)
         .def("__enter__", &detail::OstreamRedirect::enter)
-        .def("__exit__", [](detail::OstreamRedirect &self_, args) { self_.exit(); });
+        .def("__exit__", [](detail::OstreamRedirect &self_, const args &) { self_.exit(); });
 }
 
 PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)

include/pybind11/pybind11.h

@@ -1629,12 +1629,13 @@ struct enum_base {
         auto static_property = handle((PyObject *) get_internals().static_property_type);
 
         m_base.attr("__repr__") = cpp_function(
-            [](object arg) -> str {
+            [](const object &arg) -> str {
                 handle type = type::handle_of(arg);
                 object type_name = type.attr("__name__");
                 return pybind11::str("<{}.{}: {}>").format(type_name, enum_name(arg), int_(arg));
-            }, name("__repr__"), is_method(m_base)
-        );
+            },
+            name("__repr__"),
+            is_method(m_base));
 
         m_base.attr("name") = property(cpp_function(&enum_name, name("name"), is_method(m_base)));
 
@@ -1718,8 +1719,10 @@ struct enum_base {
                 PYBIND11_ENUM_OP_CONV("__ror__",  a |  b);
                 PYBIND11_ENUM_OP_CONV("__xor__",  a ^  b);
                 PYBIND11_ENUM_OP_CONV("__rxor__", a ^  b);
-                m_base.attr("__invert__") = cpp_function(
-                    [](object arg) { return ~(int_(arg)); }, name("__invert__"), is_method(m_base));
+                m_base.attr("__invert__")
+                    = cpp_function([](const object &arg) { return ~(int_(arg)); },
+                                   name("__invert__"),
+                                   is_method(m_base));
             }
         } else {
             PYBIND11_ENUM_OP_STRICT("__eq__",  int_(a).equal(int_(b)), return false);
@@ -1740,10 +1743,10 @@ struct enum_base {
         #undef PYBIND11_ENUM_OP_STRICT
 
         m_base.attr("__getstate__") = cpp_function(
-            [](object arg) { return int_(arg); }, name("__getstate__"), is_method(m_base));
+            [](const object &arg) { return int_(arg); }, name("__getstate__"), is_method(m_base));
 
         m_base.attr("__hash__") = cpp_function(
-            [](object arg) { return int_(arg); }, name("__hash__"), is_method(m_base));
+            [](const object &arg) { return int_(arg); }, name("__hash__"), is_method(m_base));
     }
 
     PYBIND11_NOINLINE void value(char const* name_, object value, const char *doc = nullptr) {

tests/local_bindings.h

@@ -1,4 +1,6 @@
 #pragma once
+#include <utility>
+
 #include "pybind11_tests.h"
 
 /// Simple class used to test py::local:
@@ -54,7 +56,7 @@ py::class_<T> bind_local(Args && ...args) {
 namespace pets {
 class Pet {
 public:
-    Pet(std::string name) : name_(name) {}
+    Pet(std::string name) : name_(std::move(name)) {}
     std::string name_;
     const std::string &name() const { return name_; }
 };

tests/object.h

@@ -81,7 +81,7 @@ template <typename T> class ref {
     }
 
     /// Move constructor
-    ref(ref &&r) : m_ptr(r.m_ptr) {
+    ref(ref &&r) noexcept : m_ptr(r.m_ptr) {
         r.m_ptr = nullptr;
 
         print_move_created(this, "with pointer", m_ptr); track_move_created((ref_tag*) this);
@@ -96,7 +96,7 @@ template <typename T> class ref {
     }
 
     /// Move another reference into the current one
-    ref& operator=(ref&& r) {
+    ref &operator=(ref &&r) noexcept {
         print_move_assigned(this, "pointer", r.m_ptr); track_move_assigned((ref_tag*) this);
 
         if (*this == r)

tests/pybind11_cross_module_tests.cpp

@@ -104,6 +104,8 @@ PYBIND11_MODULE(pybind11_cross_module_tests, m) {
     m.def("return_self", [](LocalVec *v) { return v; });
     m.def("return_copy", [](const LocalVec &v) { return LocalVec(v); });
 
+    // Changing this broke things with pygrep. TODO fix
+    // NOLINTNEXTLINE
     class Dog : public pets::Pet { public: Dog(std::string name) : Pet(name) {}; };
     py::class_<pets::Pet>(m, "Pet", py::module_local())
         .def("name", &pets::Pet::name);
@@ -126,6 +128,7 @@ PYBIND11_MODULE(pybind11_cross_module_tests, m) {
     // test_missing_header_message
     // The main module already includes stl.h, but we need to test the error message
     // which appears when this header is missing.
+    // NOLINTNEXTLINE(performance-unnecessary-value-param)
     m.def("missing_header_arg", [](std::vector<float>) { });
     m.def("missing_header_return", []() { return std::vector<float>(); });
 }

tests/test_buffers.cpp

@@ -27,7 +27,7 @@ TEST_SUBMODULE(buffers, m) {
             memcpy(m_data, s.m_data, sizeof(float) * (size_t) (m_rows * m_cols));
         }
 
-        Matrix(Matrix &&s) : m_rows(s.m_rows), m_cols(s.m_cols), m_data(s.m_data) {
+        Matrix(Matrix &&s) noexcept : m_rows(s.m_rows), m_cols(s.m_cols), m_data(s.m_data) {
             print_move_created(this);
             s.m_rows = 0;
             s.m_cols = 0;
@@ -49,7 +49,7 @@ TEST_SUBMODULE(buffers, m) {
             return *this;
         }
 
-        Matrix &operator=(Matrix &&s) {
+        Matrix &operator=(Matrix &&s) noexcept {
             print_move_assigned(this, std::to_string(m_rows) + "x" + std::to_string(m_cols) + " matrix");
             if (&s != this) {
                 delete[] m_data;
@@ -79,6 +79,7 @@ TEST_SUBMODULE(buffers, m) {
     py::class_<Matrix>(m, "Matrix", py::buffer_protocol())
         .def(py::init<py::ssize_t, py::ssize_t>())
         /// Construct from a buffer
+        // NOLINTNEXTLINE(performance-unnecessary-value-param)
         .def(py::init([](py::buffer const b) {
             py::buffer_info info = b.request();
             if (info.format != py::format_descriptor<float>::format() || info.ndim != 2)
@@ -89,31 +90,31 @@ TEST_SUBMODULE(buffers, m) {
             return v;
         }))
 
-       .def("rows", &Matrix::rows)
-       .def("cols", &Matrix::cols)
+        .def("rows", &Matrix::rows)
+        .def("cols", &Matrix::cols)
 
         /// Bare bones interface
-       .def("__getitem__", [](const Matrix &m, std::pair<py::ssize_t, py::ssize_t> i) {
-            if (i.first >= m.rows() || i.second >= m.cols())
-                throw py::index_error();
-            return m(i.first, i.second);
-        })
-       .def("__setitem__", [](Matrix &m, std::pair<py::ssize_t, py::ssize_t> i, float v) {
-            if (i.first >= m.rows() || i.second >= m.cols())
-                throw py::index_error();
-            m(i.first, i.second) = v;
-        })
-       /// Provide buffer access
-       .def_buffer([](Matrix &m) -> py::buffer_info {
+        .def("__getitem__",
+             [](const Matrix &m, std::pair<py::ssize_t, py::ssize_t> i) {
+                 if (i.first >= m.rows() || i.second >= m.cols())
+                     throw py::index_error();
+                 return m(i.first, i.second);
+             })
+        .def("__setitem__",
+             [](Matrix &m, std::pair<py::ssize_t, py::ssize_t> i, float v) {
+                 if (i.first >= m.rows() || i.second >= m.cols())
+                     throw py::index_error();
+                 m(i.first, i.second) = v;
+             })
+        /// Provide buffer access
+        .def_buffer([](Matrix &m) -> py::buffer_info {
             return py::buffer_info(
                 m.data(),                               /* Pointer to buffer */
                 { m.rows(), m.cols() },                 /* Buffer dimensions */
                 { sizeof(float) * size_t(m.cols()),     /* Strides (in bytes) for each index */
                   sizeof(float) }
             );
-        })
-        ;
-
+        });
 
     // test_inherited_protocol
     class SquareMatrix : public Matrix {
@@ -208,7 +209,5 @@ TEST_SUBMODULE(buffers, m) {
         })
         ;
 
-    m.def("get_buffer_info", [](py::buffer buffer) {
-        return buffer.request();
-    });
+    m.def("get_buffer_info", [](const py::buffer &buffer) { return buffer.request(); });
 }

tests/test_builtin_casters.cpp

@@ -30,7 +30,11 @@ class type_caster<ConstRefCasted> {
   // cast operator.
   bool load(handle, bool) { return true; }
 
-  operator ConstRefCasted&&() { value = {1}; return std::move(value); }
+  operator ConstRefCasted &&() {
+      value = {1};
+      // NOLINTNEXTLINE(performance-move-const-arg)
+      return std::move(value);
+  }
   operator ConstRefCasted&() { value = {2}; return value; }
   operator ConstRefCasted*() { value = {3}; return &value; }
 
@@ -101,6 +105,7 @@ TEST_SUBMODULE(builtin_casters, m) {
 
     // test_bytes_to_string
     m.def("strlen", [](char *s) { return strlen(s); });
+    // NOLINTNEXTLINE(performance-unnecessary-value-param)
     m.def("string_length", [](std::string s) { return s.length(); });
 
 #ifdef PYBIND11_HAS_U8STRING
@@ -146,6 +151,7 @@ TEST_SUBMODULE(builtin_casters, m) {
     m.def("int_passthrough_noconvert", [](int arg) { return arg; }, py::arg{}.noconvert());
 
     // test_tuple
+    // NOLINTNEXTLINE(performance-unnecessary-value-param)
     m.def("pair_passthrough", [](std::pair<bool, std::string> input) {
         return std::make_pair(input.second, input.first);
     }, "Return a pair in reversed order");
@@ -177,10 +183,13 @@ TEST_SUBMODULE(builtin_casters, m) {
 
     // test_none_deferred
     m.def("defer_none_cstring", [](char *) { return false; });
+    // NOLINTNEXTLINE(performance-unnecessary-value-param)
     m.def("defer_none_cstring", [](py::none) { return true; });
     m.def("defer_none_custom", [](UserType *) { return false; });
+    // NOLINTNEXTLINE(performance-unnecessary-value-param)
     m.def("defer_none_custom", [](py::none) { return true; });
     m.def("nodefer_none_void", [](void *) { return true; });
+    // NOLINTNEXTLINE(performance-unnecessary-value-param)
     m.def("nodefer_none_void", [](py::none) { return false; });
 
     // test_void_caster
@@ -231,6 +240,7 @@ TEST_SUBMODULE(builtin_casters, m) {
     }, "copy"_a);
 
     m.def("refwrap_iiw", [](const IncType &w) { return w.value(); });
+    // NOLINTNEXTLINE(performance-unnecessary-value-param)
     m.def("refwrap_call_iiw", [](IncType &w, py::function f) {
         py::list l;
         l.append(f(std::ref(w)));

tests/test_callbacks.cpp

@@ -17,8 +17,8 @@ int dummy_function(int i) { return i + 1; }
 
 TEST_SUBMODULE(callbacks, m) {
     // test_callbacks, test_function_signatures
-    m.def("test_callback1", [](py::object func) { return func(); });
-    m.def("test_callback2", [](py::object func) { return func("Hello", 'x', true, 5); });
+    m.def("test_callback1", [](const py::object &func) { return func(); });
+    m.def("test_callback2", [](const py::object &func) { return func("Hello", 'x', true, 5); });
     m.def("test_callback3", [](const std::function<int(int)> &func) {
         return "func(43) = " + std::to_string(func(43)); });
     m.def("test_callback4", []() -> std::function<int(int)> { return [](int i) { return i+1; }; });
@@ -27,51 +27,48 @@ TEST_SUBMODULE(callbacks, m) {
     });
 
     // test_keyword_args_and_generalized_unpacking
-    m.def("test_tuple_unpacking", [](py::function f) {
+    m.def("test_tuple_unpacking", [](const py::function &f) {
         auto t1 = py::make_tuple(2, 3);
         auto t2 = py::make_tuple(5, 6);
         return f("positional", 1, *t1, 4, *t2);
     });
 
-    m.def("test_dict_unpacking", [](py::function f) {
+    m.def("test_dict_unpacking", [](const py::function &f) {
         auto d1 = py::dict("key"_a="value", "a"_a=1);
         auto d2 = py::dict();
         auto d3 = py::dict("b"_a=2);
         return f("positional", 1, **d1, **d2, **d3);
     });
 
-    m.def("test_keyword_args", [](py::function f) {
-        return f("x"_a=10, "y"_a=20);
-    });
+    m.def("test_keyword_args", [](const py::function &f) { return f("x"_a = 10, "y"_a = 20); });
 
-    m.def("test_unpacking_and_keywords1", [](py::function f) {
+    m.def("test_unpacking_and_keywords1", [](const py::function &f) {
         auto args = py::make_tuple(2);
         auto kwargs = py::dict("d"_a=4);
         return f(1, *args, "c"_a=3, **kwargs);
     });
 
-    m.def("test_unpacking_and_keywords2", [](py::function f) {
+    m.def("test_unpacking_and_keywords2", [](const py::function &f) {
         auto kwargs1 = py::dict("a"_a=1);
         auto kwargs2 = py::dict("c"_a=3, "d"_a=4);
         return f("positional", *py::make_tuple(1), 2, *py::make_tuple(3, 4), 5,
                  "key"_a="value", **kwargs1, "b"_a=2, **kwargs2, "e"_a=5);
     });
 
-    m.def("test_unpacking_error1", [](py::function f) {
+    m.def("test_unpacking_error1", [](const py::function &f) {
         auto kwargs = py::dict("x"_a=3);
         return f("x"_a=1, "y"_a=2, **kwargs); // duplicate ** after keyword
     });
 
-    m.def("test_unpacking_error2", [](py::function f) {
+    m.def("test_unpacking_error2", [](const py::function &f) {
         auto kwargs = py::dict("x"_a=3);
         return f(**kwargs, "x"_a=1); // duplicate keyword after **
     });
 
-    m.def("test_arg_conversion_error1", [](py::function f) {
-        f(234, UnregisteredType(), "kw"_a=567);
-    });
+    m.def("test_arg_conversion_error1",
+          [](const py::function &f) { f(234, UnregisteredType(), "kw"_a = 567); });
 
-    m.def("test_arg_conversion_error2", [](py::function f) {
+    m.def("test_arg_conversion_error2", [](const py::function &f) {
         f(234, "expected_name"_a=UnregisteredType(), "kw"_a=567);
     });
 
@@ -80,7 +77,7 @@ TEST_SUBMODULE(callbacks, m) {
         Payload() { print_default_created(this); }
         ~Payload() { print_destroyed(this); }
         Payload(const Payload &) { print_copy_created(this); }
-        Payload(Payload &&) { print_move_created(this); }
+        Payload(Payload &&) noexcept { print_move_created(this); }
     };
     // Export the payload constructor statistics for testing purposes:
     m.def("payload_cstats", &ConstructorStats::get<Payload>);
@@ -127,16 +124,17 @@ TEST_SUBMODULE(callbacks, m) {
         virtual ~AbstractBase() {};  // NOLINT(modernize-use-equals-default)
         virtual unsigned int func() = 0;
     };
-    m.def("func_accepting_func_accepting_base", [](std::function<double(AbstractBase&)>) { });
+    m.def("func_accepting_func_accepting_base",
+          [](const std::function<double(AbstractBase &)> &) {});
 
     struct MovableObject {
         bool valid = true;
 
         MovableObject() = default;
         MovableObject(const MovableObject &) = default;
         MovableObject &operator=(const MovableObject &) = default;
-        MovableObject(MovableObject &&o) : valid(o.valid) { o.valid = false; }
-        MovableObject &operator=(MovableObject &&o) {
+        MovableObject(MovableObject &&o) noexcept : valid(o.valid) { o.valid = false; }
+        MovableObject &operator=(MovableObject &&o) noexcept {
             valid = o.valid;
             o.valid = false;
             return *this;
@@ -145,7 +143,7 @@ TEST_SUBMODULE(callbacks, m) {
     py::class_<MovableObject>(m, "MovableObject");
 
     // test_movable_object
-    m.def("callback_with_movable", [](std::function<void(MovableObject &)> f) {
+    m.def("callback_with_movable", [](const std::function<void(MovableObject &)> &f) {
         auto x = MovableObject();
         f(x); // lvalue reference shouldn't move out object
         return x.valid; // must still return `true`
@@ -159,7 +157,7 @@ TEST_SUBMODULE(callbacks, m) {
 
     // test async Python callbacks
     using callback_f = std::function<void(int)>;
-    m.def("test_async_callback", [](callback_f f, py::list work) {
+    m.def("test_async_callback", [](const callback_f &f, const py::list &work) {
         // make detached thread that calls `f` with piece of work after a little delay
         auto start_f = [f](int j) {
             auto invoke_f = [f, j] {
@@ -175,7 +173,7 @@ TEST_SUBMODULE(callbacks, m) {
             start_f(py::cast<int>(i));
     });
 
-    m.def("callback_num_times", [](py::function f, std::size_t num) {
+    m.def("callback_num_times", [](const py::function &f, std::size_t num) {
         for (std::size_t i = 0; i < num; i++) {
             f();
         }