From 7a64b8adcc7d74b7f6ce601db8b47a00e4cf107c Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Mon, 12 Jul 2021 14:10:46 -0400 Subject: [PATCH 1/5] docs: fix script issues for changelog compilation (#3100) [skip ci] --- tools/make_changelog.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tools/make_changelog.py b/tools/make_changelog.py index 609ce2f19..4e8fbf5b0 100755 --- a/tools/make_changelog.py +++ b/tools/make_changelog.py @@ -27,7 +27,10 @@ print() api = ghapi.all.GhApi(owner="pybind", repo="pybind11") -issues = api.issues.list_for_repo(labels="needs changelog", state="closed") +issues_pages = ghapi.page.paged( + api.issues.list_for_repo, labels="needs changelog", state="closed" +) +issues = (issue for page in issues_pages for issue in page) missing = [] for issue in issues: @@ -41,7 +44,7 @@ for issue in issues: msg += f"\n `#{issue.number} <{issue.html_url}>`_" - print(Syntax(msg, "rst", theme="ansi_light")) + print(Syntax(msg, "rst", theme="ansi_light", word_wrap=True)) print() else: From 2d468697d9c1a6209c2ccf13f3b57ba548230655 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 12 Jul 2021 13:10:28 -0700 Subject: [PATCH 2/5] NOLINT reduction (#3096) * Copying from prework_no_rst branch (PR #3087): test_numpy_array.cpp, test_stl.cpp * Manual changes reducing NOLINTs. * clang-format-diff.py * Minor adjustment to avoid MSVC warning C4702: unreachable code --- tests/pybind11_cross_module_tests.cpp | 3 +- tests/test_buffers.cpp | 3 +- tests/test_builtin_casters.cpp | 15 ++-- tests/test_class.cpp | 9 +- tests/test_copy_move.cpp | 4 +- tests/test_eigen.cpp | 52 +++++------ tests/test_exceptions.cpp | 9 +- tests/test_kwargs_and_defaults.cpp | 10 +-- tests/test_methods_and_attributes.cpp | 13 ++- tests/test_multiple_inheritance.cpp | 3 +- tests/test_numpy_array.cpp | 122 ++++++++++---------------- tests/test_numpy_vectorize.cpp | 24 +++-- tests/test_pytypes.cpp | 60 +++++-------- tests/test_stl.cpp | 14 ++- 14 files changed, 138 insertions(+), 203 deletions(-) diff --git a/tests/pybind11_cross_module_tests.cpp b/tests/pybind11_cross_module_tests.cpp index 27a349d23..e79701c6c 100644 --- a/tests/pybind11_cross_module_tests.cpp +++ b/tests/pybind11_cross_module_tests.cpp @@ -132,7 +132,6 @@ 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) { }); + m.def("missing_header_arg", [](const std::vector &) {}); m.def("missing_header_return", []() { return std::vector(); }); } diff --git a/tests/test_buffers.cpp b/tests/test_buffers.cpp index 9902c1084..15dd4f1c7 100644 --- a/tests/test_buffers.cpp +++ b/tests/test_buffers.cpp @@ -79,8 +79,7 @@ TEST_SUBMODULE(buffers, m) { py::class_(m, "Matrix", py::buffer_protocol()) .def(py::init()) /// Construct from a buffer - // NOLINTNEXTLINE(performance-unnecessary-value-param) - .def(py::init([](py::buffer const b) { + .def(py::init([](const py::buffer &b) { py::buffer_info info = b.request(); if (info.format != py::format_descriptor::format() || info.ndim != 2) throw std::runtime_error("Incompatible buffer format!"); diff --git a/tests/test_builtin_casters.cpp b/tests/test_builtin_casters.cpp index caafb0631..51e410cad 100644 --- a/tests/test_builtin_casters.cpp +++ b/tests/test_builtin_casters.cpp @@ -105,8 +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(); }); + m.def("string_length", [](const std::string &s) { return s.length(); }); #ifdef PYBIND11_HAS_U8STRING m.attr("has_u8string") = true; @@ -185,14 +184,11 @@ 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_cstring", [](const 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("defer_none_custom", [](const 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; }); + m.def("nodefer_none_void", [](const py::none &) { return false; }); // test_void_caster m.def("load_nullptr_t", [](std::nullptr_t) {}); // not useful, but it should still compile @@ -242,8 +238,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) { + m.def("refwrap_call_iiw", [](IncType &w, const py::function &f) { py::list l; l.append(f(std::ref(w))); l.append(f(std::cref(w))); diff --git a/tests/test_class.cpp b/tests/test_class.cpp index bcc73ef0b..73de2a61a 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -131,8 +131,7 @@ TEST_SUBMODULE(class_, m) { m.def("return_none", []() -> BaseClass* { return nullptr; }); // test_isinstance - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("check_instances", [](py::list l) { + m.def("check_instances", [](const py::list &l) { return py::make_tuple( py::isinstance(l[0]), py::isinstance(l[1]), @@ -217,8 +216,7 @@ TEST_SUBMODULE(class_, m) { py::implicitly_convertible(); m.def("implicitly_convert_argument", [](const ConvertibleFromUserType &r) { return r.i; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("implicitly_convert_variable", [](py::object o) { + m.def("implicitly_convert_variable", [](const py::object &o) { // `o` is `UserType` and `r` is a reference to a temporary created by implicit // conversion. This is valid when called inside a bound function because the temp // object is attached to the same life support system as the arguments. @@ -396,8 +394,7 @@ TEST_SUBMODULE(class_, m) { struct StringWrapper { std::string str; }; m.def("test_error_after_conversions", [](int) {}); m.def("test_error_after_conversions", - // NOLINTNEXTLINE(performance-unnecessary-value-param) - [](StringWrapper) -> NotRegistered { return {}; }); + [](const StringWrapper &) -> NotRegistered { return {}; }); py::class_(m, "StringWrapper").def(py::init()); py::implicitly_convertible(); diff --git a/tests/test_copy_move.cpp b/tests/test_copy_move.cpp index 05caf28ff..859da9df7 100644 --- a/tests/test_copy_move.cpp +++ b/tests/test_copy_move.cpp @@ -126,7 +126,7 @@ TEST_SUBMODULE(copy_move_policies, m) { // test_move_and_copy_casts // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("move_and_copy_casts", [](py::object o) { + m.def("move_and_copy_casts", [](const py::object &o) { int r = 0; r += py::cast(o).value; /* moves */ r += py::cast(o).value; /* moves */ @@ -141,8 +141,10 @@ TEST_SUBMODULE(copy_move_policies, m) { // test_move_and_copy_loads m.def("move_only", [](MoveOnlyInt m) { return m.value; }); + // Changing this breaks the existing test: needs careful review. // NOLINTNEXTLINE(performance-unnecessary-value-param) m.def("move_or_copy", [](MoveOrCopyInt m) { return m.value; }); + // Changing this breaks the existing test: needs careful review. // NOLINTNEXTLINE(performance-unnecessary-value-param) m.def("copy_only", [](CopyOnlyInt m) { return m.value; }); m.def("move_pair", [](std::pair p) { diff --git a/tests/test_eigen.cpp b/tests/test_eigen.cpp index a24bf273d..ba74f5815 100644 --- a/tests/test_eigen.cpp +++ b/tests/test_eigen.cpp @@ -98,12 +98,13 @@ TEST_SUBMODULE(eigen, m) { // test_eigen_ref_to_python // Different ways of passing via Eigen::Ref; the first and second are the Eigen-recommended - // NOLINTNEXTLINE (performance-unnecessary-value-param) - m.def("cholesky1", [](Eigen::Ref x) -> Eigen::MatrixXd { return x.llt().matrixL(); }); + m.def("cholesky1", + [](const Eigen::Ref &x) -> Eigen::MatrixXd { return x.llt().matrixL(); }); m.def("cholesky2", [](const Eigen::Ref &x) -> Eigen::MatrixXd { return x.llt().matrixL(); }); m.def("cholesky3", [](const Eigen::Ref &x) -> Eigen::MatrixXd { return x.llt().matrixL(); }); - // NOLINTNEXTLINE (performance-unnecessary-value-param) - m.def("cholesky4", [](Eigen::Ref x) -> Eigen::MatrixXd { return x.llt().matrixL(); }); + m.def("cholesky4", [](const Eigen::Ref &x) -> Eigen::MatrixXd { + return x.llt().matrixL(); + }); // test_eigen_ref_mutators // Mutators: these add some value to the given element using Eigen, but Eigen should be mapping into @@ -248,12 +249,9 @@ TEST_SUBMODULE(eigen, m) { m.def("fixed_copy_r", [](const FixedMatrixR &m) -> FixedMatrixR { return m; }); m.def("fixed_copy_c", [](const FixedMatrixC &m) -> FixedMatrixC { return m; }); // test_mutator_descriptors - // NOLINTNEXTLINE (performance-unnecessary-value-param) - m.def("fixed_mutator_r", [](Eigen::Ref) {}); - // NOLINTNEXTLINE (performance-unnecessary-value-param) - m.def("fixed_mutator_c", [](Eigen::Ref) {}); - // NOLINTNEXTLINE (performance-unnecessary-value-param) - m.def("fixed_mutator_a", [](py::EigenDRef) {}); + m.def("fixed_mutator_r", [](const Eigen::Ref &) {}); + m.def("fixed_mutator_c", [](const Eigen::Ref &) {}); + m.def("fixed_mutator_a", [](const py::EigenDRef &) {}); // test_dense m.def("dense_r", [mat]() -> DenseMatrixR { return DenseMatrixR(mat); }); m.def("dense_c", [mat]() -> DenseMatrixC { return DenseMatrixC(mat); }); @@ -284,9 +282,10 @@ TEST_SUBMODULE(eigen, m) { // that would allow copying (if types or strides don't match) for comparison: m.def("get_elem", &get_elem); // Now this alternative that calls the tells pybind to fail rather than copy: - // NOLINTNEXTLINE (performance-unnecessary-value-param) - m.def("get_elem_nocopy", [](Eigen::Ref m) -> double { return get_elem(m); }, - py::arg{}.noconvert()); + m.def( + "get_elem_nocopy", + [](const Eigen::Ref &m) -> double { return get_elem(m); }, + py::arg{}.noconvert()); // Also test a row-major-only no-copy const ref: m.def("get_elem_rm_nocopy", [](Eigen::Ref> &m) -> long { return m(2, 1); }, py::arg{}.noconvert()); @@ -301,20 +300,22 @@ TEST_SUBMODULE(eigen, m) { // test_issue1105 // Issue #1105: when converting from a numpy two-dimensional (Nx1) or (1xN) value into a dense // eigen Vector or RowVector, the argument would fail to load because the numpy copy would - // fail: numpy won't broadcast a Nx1 into a 1-dimensional vector. NOLINTNEXTLINE - // NOLINTNEXTLINE (performance-unnecessary-value-param) - m.def("iss1105_col", [](Eigen::VectorXd) { return true; }); - // NOLINTNEXTLINE (performance-unnecessary-value-param) - m.def("iss1105_row", [](Eigen::RowVectorXd) { return true; }); + // fail: numpy won't broadcast a Nx1 into a 1-dimensional vector. + m.def("iss1105_col", [](const Eigen::VectorXd &) { return true; }); + m.def("iss1105_row", [](const Eigen::RowVectorXd &) { return true; }); // test_named_arguments // Make sure named arguments are working properly: - // NOLINTNEXTLINE (performance-unnecessary-value-param) - m.def("matrix_multiply", [](const py::EigenDRef A, const py::EigenDRef B) - -> Eigen::MatrixXd { - if (A.cols() != B.rows()) throw std::domain_error("Nonconformable matrices!"); - return A * B; - }, py::arg("A"), py::arg("B")); + m.def( + "matrix_multiply", + [](const py::EigenDRef &A, + const py::EigenDRef &B) -> Eigen::MatrixXd { + if (A.cols() != B.rows()) + throw std::domain_error("Nonconformable matrices!"); + return A * B; + }, + py::arg("A"), + py::arg("B")); // test_custom_operator_new py::class_(m, "CustomOperatorNew") @@ -326,8 +327,7 @@ TEST_SUBMODULE(eigen, m) { // In case of a failure (the caster's temp array does not live long enough), creating // a new array (np.ones(10)) increases the chances that the temp array will be garbage // collected and/or that its memory will be overridden with different values. - // NOLINTNEXTLINE (performance-unnecessary-value-param) - m.def("get_elem_direct", [](Eigen::Ref v) { + m.def("get_elem_direct", [](const Eigen::Ref &v) { py::module_::import("numpy").attr("ones")(10); return v(5); }); diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index d34f1a94e..f7bacd07e 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -201,17 +201,16 @@ TEST_SUBMODULE(exceptions, m) { throw py::error_already_set(); }); - // Changing this broke things. Don't know why - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("python_call_in_destructor", [](py::dict d) { + m.def("python_call_in_destructor", [](const py::dict &d) { + bool retval = false; try { PythonCallInDestructor set_dict_in_destructor(d); PyErr_SetString(PyExc_ValueError, "foo"); throw py::error_already_set(); } catch (const py::error_already_set&) { - return true; + retval = true; } - return false; + return retval; }); m.def("python_alreadyset_in_destructor", [](const py::str &s) { diff --git a/tests/test_kwargs_and_defaults.cpp b/tests/test_kwargs_and_defaults.cpp index 3e078bd9b..63332d32e 100644 --- a/tests/test_kwargs_and_defaults.cpp +++ b/tests/test_kwargs_and_defaults.cpp @@ -65,9 +65,10 @@ TEST_SUBMODULE(kwargs_and_defaults, m) { #endif m.def("arg_refcount_h", [](py::handle h) { GC_IF_NEEDED; return h.ref_count(); }); m.def("arg_refcount_h", [](py::handle h, py::handle, py::handle) { GC_IF_NEEDED; return h.ref_count(); }); - // TODO replace the following nolints as appropriate - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("arg_refcount_o", [](py::object o) { GC_IF_NEEDED; return o.ref_count(); }); + m.def("arg_refcount_o", [](const py::object &o) { + GC_IF_NEEDED; + return o.ref_count(); + }); m.def("args_refcount", [](py::args a) { GC_IF_NEEDED; py::tuple t(a.size()); @@ -76,8 +77,7 @@ TEST_SUBMODULE(kwargs_and_defaults, m) { t[i] = (int) Py_REFCNT(PyTuple_GET_ITEM(a.ptr(), static_cast(i))); return t; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("mixed_args_refcount", [](py::object o, py::args a) { + m.def("mixed_args_refcount", [](const py::object &o, py::args a) { GC_IF_NEEDED; py::tuple t(a.size() + 1); t[0] = o.ref_count(); diff --git a/tests/test_methods_and_attributes.cpp b/tests/test_methods_and_attributes.cpp index 67ee117c6..6e5999f0c 100644 --- a/tests/test_methods_and_attributes.cpp +++ b/tests/test_methods_and_attributes.cpp @@ -294,20 +294,17 @@ TEST_SUBMODULE(methods_and_attributes, m) { "static_rw_func", py::cpp_function(static_get2, rvp_copy), static_set2) // test_property_rvalue_policy .def_property_readonly("rvalue", &TestPropRVP::get_rvalue) - // NOLINTNEXTLINE(performance-unnecessary-value-param) - .def_property_readonly_static("static_rvalue", [](py::object) { return UserType(1); }); + .def_property_readonly_static("static_rvalue", + [](const py::object &) { return UserType(1); }); // test_metaclass_override struct MetaclassOverride { }; py::class_(m, "MetaclassOverride", py::metaclass((PyObject *) &PyType_Type)) - // NOLINTNEXTLINE(performance-unnecessary-value-param) - .def_property_readonly_static("readonly", [](py::object) { return 1; }); + .def_property_readonly_static("readonly", [](const py::object &) { return 1; }); // test_overload_ordering - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("overload_order", [](std::string) { return 1; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("overload_order", [](std::string) { return 2; }); + m.def("overload_order", [](const std::string &) { return 1; }); + m.def("overload_order", [](const std::string &) { return 2; }); m.def("overload_order", [](int) { return 3; }); m.def("overload_order", [](int) { return 4; }, py::prepend{}); diff --git a/tests/test_multiple_inheritance.cpp b/tests/test_multiple_inheritance.cpp index f4acf7861..d6b24d34d 100644 --- a/tests/test_multiple_inheritance.cpp +++ b/tests/test_multiple_inheritance.cpp @@ -141,8 +141,7 @@ TEST_SUBMODULE(multiple_inheritance, m) { .def(py::init()); m.def("bar_base2a", [](Base2a *b) { return b->bar(); }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("bar_base2a_sharedptr", [](std::shared_ptr b) { return b->bar(); }); + m.def("bar_base2a_sharedptr", [](const std::shared_ptr &b) { return b->bar(); }); // test_mi_unaligned_base // test_mi_base_return diff --git a/tests/test_numpy_array.cpp b/tests/test_numpy_array.cpp index a6c7ae8d7..5c22a3d25 100644 --- a/tests/test_numpy_array.cpp +++ b/tests/test_numpy_array.cpp @@ -226,8 +226,7 @@ TEST_SUBMODULE(numpy_array, sm) { return py::isinstance(std::move(yes)) && !py::isinstance(std::move(no)); }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("isinstance_typed", [](py::object o) { + sm.def("isinstance_typed", [](const py::object &o) { return py::isinstance>(o) && !py::isinstance>(o); }); @@ -248,60 +247,47 @@ TEST_SUBMODULE(numpy_array, sm) { }); // test_overload_resolution - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("overloaded", [](py::array_t) { return "double"; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("overloaded", [](py::array_t) { return "float"; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("overloaded", [](py::array_t) { return "int"; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("overloaded", [](py::array_t) { return "unsigned short"; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("overloaded", [](py::array_t) { return "long long"; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("overloaded", [](py::array_t>) { return "double complex"; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("overloaded", [](py::array_t>) { return "float complex"; }); + sm.def("overloaded", [](const py::array_t &) { return "double"; }); + sm.def("overloaded", [](const py::array_t &) { return "float"; }); + sm.def("overloaded", [](const py::array_t &) { return "int"; }); + sm.def("overloaded", [](const py::array_t &) { return "unsigned short"; }); + sm.def("overloaded", [](const py::array_t &) { return "long long"; }); + sm.def("overloaded", + [](const py::array_t> &) { return "double complex"; }); + sm.def("overloaded", [](const py::array_t> &) { return "float complex"; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("overloaded2", [](py::array_t>) { return "double complex"; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("overloaded2", [](py::array_t) { return "double"; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("overloaded2", [](py::array_t>) { return "float complex"; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("overloaded2", [](py::array_t) { return "float"; }); + sm.def("overloaded2", + [](const py::array_t> &) { return "double complex"; }); + sm.def("overloaded2", [](const py::array_t &) { return "double"; }); + sm.def("overloaded2", + [](const py::array_t> &) { return "float complex"; }); + sm.def("overloaded2", [](const py::array_t &) { return "float"; }); // [workaround(intel)] ICC 20/21 breaks with py::arg().stuff, using py::arg{}.stuff works. // Only accept the exact types: - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("overloaded3", [](py::array_t) { return "int"; }, py::arg{}.noconvert()); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("overloaded3", [](py::array_t) { return "double"; }, py::arg{}.noconvert()); + sm.def( + "overloaded3", [](const py::array_t &) { return "int"; }, py::arg{}.noconvert()); + sm.def( + "overloaded3", + [](const py::array_t &) { return "double"; }, + py::arg{}.noconvert()); // Make sure we don't do unsafe coercion (e.g. float to int) when not using forcecast, but // rather that float gets converted via the safe (conversion to double) overload: - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("overloaded4", [](py::array_t) { return "long long"; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("overloaded4", [](py::array_t) { return "double"; }); + sm.def("overloaded4", [](const py::array_t &) { return "long long"; }); + sm.def("overloaded4", [](const py::array_t &) { return "double"; }); // But we do allow conversion to int if forcecast is enabled (but only if no overload matches // without conversion) - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("overloaded5", [](py::array_t) { return "unsigned int"; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("overloaded5", [](py::array_t) { return "double"; }); + sm.def("overloaded5", [](const py::array_t &) { return "unsigned int"; }); + sm.def("overloaded5", [](const py::array_t &) { return "double"; }); // test_greedy_string_overload // Issue 685: ndarray shouldn't go to std::string overload - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("issue685", [](std::string) { return "string"; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("issue685", [](py::array) { return "array"; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("issue685", [](py::object) { return "other"; }); + sm.def("issue685", [](const std::string &) { return "string"; }); + sm.def("issue685", [](const py::array &) { return "array"; }); + sm.def("issue685", [](const py::object &) { return "other"; }); // test_array_unchecked_fixed_dims sm.def("proxy_add2", [](py::array_t a, double v) { @@ -424,73 +410,53 @@ TEST_SUBMODULE(numpy_array, sm) { // test_argument_conversions sm.def( - "accept_double", - // NOLINTNEXTLINE(performance-unnecessary-value-param) - [](py::array_t) {}, - py::arg("a")); + "accept_double", [](const py::array_t &) {}, py::arg("a")); sm.def( "accept_double_forcecast", - // NOLINTNEXTLINE(performance-unnecessary-value-param) - [](py::array_t) {}, + [](const py::array_t &) {}, py::arg("a")); sm.def( "accept_double_c_style", - // NOLINTNEXTLINE(performance-unnecessary-value-param) - [](py::array_t) {}, + [](const py::array_t &) {}, py::arg("a")); sm.def( "accept_double_c_style_forcecast", - // NOLINTNEXTLINE(performance-unnecessary-value-param) - [](py::array_t) {}, + [](const py::array_t &) {}, py::arg("a")); sm.def( "accept_double_f_style", - // NOLINTNEXTLINE(performance-unnecessary-value-param) - [](py::array_t) {}, + [](const py::array_t &) {}, py::arg("a")); sm.def( "accept_double_f_style_forcecast", - // NOLINTNEXTLINE(performance-unnecessary-value-param) - [](py::array_t) {}, + [](const py::array_t &) {}, py::arg("a")); sm.def( - "accept_double_noconvert", - // NOLINTNEXTLINE(performance-unnecessary-value-param) - [](py::array_t) {}, - "a"_a.noconvert()); + "accept_double_noconvert", [](const py::array_t &) {}, "a"_a.noconvert()); sm.def( "accept_double_forcecast_noconvert", - // NOLINTNEXTLINE(performance-unnecessary-value-param) - [](py::array_t) {}, + [](const py::array_t &) {}, "a"_a.noconvert()); sm.def( "accept_double_c_style_noconvert", - // NOLINTNEXTLINE(performance-unnecessary-value-param) - [](py::array_t) {}, + [](const py::array_t &) {}, "a"_a.noconvert()); sm.def( "accept_double_c_style_forcecast_noconvert", - // NOLINTNEXTLINE(performance-unnecessary-value-param) - [](py::array_t) {}, + [](const py::array_t &) {}, "a"_a.noconvert()); sm.def( "accept_double_f_style_noconvert", - // NOLINTNEXTLINE(performance-unnecessary-value-param) - [](py::array_t) {}, + [](const py::array_t &) {}, "a"_a.noconvert()); sm.def( "accept_double_f_style_forcecast_noconvert", - // NOLINTNEXTLINE(performance-unnecessary-value-param) - [](py::array_t) {}, + [](const py::array_t &) {}, "a"_a.noconvert()); // Check that types returns correct npy format descriptor - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("test_fmt_desc_float", [](py::array_t) {}); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("test_fmt_desc_double", [](py::array_t) {}); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("test_fmt_desc_const_float", [](py::array_t) {}); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - sm.def("test_fmt_desc_const_double", [](py::array_t) {}); + sm.def("test_fmt_desc_float", [](const py::array_t &) {}); + sm.def("test_fmt_desc_double", [](const py::array_t &) {}); + sm.def("test_fmt_desc_const_float", [](const py::array_t &) {}); + sm.def("test_fmt_desc_const_double", [](const py::array_t &) {}); } diff --git a/tests/test_numpy_vectorize.cpp b/tests/test_numpy_vectorize.cpp index 0be30d8e8..42fb2002e 100644 --- a/tests/test_numpy_vectorize.cpp +++ b/tests/test_numpy_vectorize.cpp @@ -40,13 +40,13 @@ TEST_SUBMODULE(numpy_vectorize, m) { // test_type_selection // NumPy function which only accepts specific data types // A lot of these no lints could be replaced with const refs, and probably should at some point. - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("selective_func", [](py::array_t) { return "Int branch taken."; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("selective_func", [](py::array_t) { return "Float branch taken."; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("selective_func", [](py::array_t, py::array::c_style>) { return "Complex float branch taken."; }); - + m.def("selective_func", + [](const py::array_t &) { return "Int branch taken."; }); + m.def("selective_func", + [](const py::array_t &) { return "Float branch taken."; }); + m.def("selective_func", [](const py::array_t, py::array::c_style> &) { + return "Complex float branch taken."; + }); // test_passthrough_arguments // Passthrough test: references and non-pod types should be automatically passed through (in the @@ -89,13 +89,9 @@ TEST_SUBMODULE(numpy_vectorize, m) { .value("c_trivial", py::detail::broadcast_trivial::c_trivial) .value("non_trivial", py::detail::broadcast_trivial::non_trivial); m.def("vectorized_is_trivial", - []( - // NOLINTNEXTLINE(performance-unnecessary-value-param) - py::array_t arg1, - // NOLINTNEXTLINE(performance-unnecessary-value-param) - py::array_t arg2, - // NOLINTNEXTLINE(performance-unnecessary-value-param) - py::array_t arg3) { + [](const py::array_t &arg1, + const py::array_t &arg2, + const py::array_t &arg3) { py::ssize_t ndim; std::vector shape; std::array buffers{ diff --git a/tests/test_pytypes.cpp b/tests/test_pytypes.cpp index 2b91af37b..6ed59aad2 100644 --- a/tests/test_pytypes.cpp +++ b/tests/test_pytypes.cpp @@ -237,8 +237,7 @@ TEST_SUBMODULE(pytypes, m) { ); }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("cast_functions", [](py::dict d) { + m.def("cast_functions", [](const py::dict &d) { // When converting between Python types, obj.cast() should be the same as T(obj) return py::dict( "bytes"_a=d["bytes"].cast(), @@ -255,25 +254,24 @@ TEST_SUBMODULE(pytypes, m) { ); }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("convert_to_pybind11_str", [](py::object o) { return py::str(o); }); + m.def("convert_to_pybind11_str", [](const py::object &o) { return py::str(o); }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("nonconverting_constructor", [](std::string type, py::object value, bool move) -> py::object { - if (type == "bytes") { - return move ? py::bytes(std::move(value)) : py::bytes(value); - } - if (type == "none") { - return move ? py::none(std::move(value)) : py::none(value); - } - if (type == "ellipsis") { - return move ? py::ellipsis(std::move(value)) : py::ellipsis(value); - } - if (type == "type") { - return move ? py::type(std::move(value)) : py::type(value); - } - throw std::runtime_error("Invalid type"); - }); + m.def("nonconverting_constructor", + [](const std::string &type, py::object value, bool move) -> py::object { + if (type == "bytes") { + return move ? py::bytes(std::move(value)) : py::bytes(value); + } + if (type == "none") { + return move ? py::none(std::move(value)) : py::none(value); + } + if (type == "ellipsis") { + return move ? py::ellipsis(std::move(value)) : py::ellipsis(value); + } + if (type == "type") { + return move ? py::type(std::move(value)) : py::type(value); + } + throw std::runtime_error("Invalid type"); + }); m.def("get_implicit_casting", []() { py::dict d; @@ -333,8 +331,7 @@ TEST_SUBMODULE(pytypes, m) { m.def("hash_function", [](py::object obj) { return py::hash(std::move(obj)); }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("test_number_protocol", [](py::object a, py::object b) { + m.def("test_number_protocol", [](const py::object &a, const py::object &b) { py::list l; l.append(a.equal(b)); l.append(a.not_equal(b)); @@ -354,10 +351,7 @@ TEST_SUBMODULE(pytypes, m) { return l; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("test_list_slicing", [](py::list a) { - return a[py::slice(0, -1, 2)]; - }); + m.def("test_list_slicing", [](const py::list &a) { return a[py::slice(0, -1, 2)]; }); // See #2361 m.def("issue2361_str_implicit_copy_none", []() { @@ -369,15 +363,10 @@ TEST_SUBMODULE(pytypes, m) { return is_this_none; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("test_memoryview_object", [](py::buffer b) { - return py::memoryview(b); - }); + m.def("test_memoryview_object", [](const py::buffer &b) { return py::memoryview(b); }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("test_memoryview_buffer_info", [](py::buffer b) { - return py::memoryview(b.request()); - }); + m.def("test_memoryview_buffer_info", + [](const py::buffer &b) { return py::memoryview(b.request()); }); m.def("test_memoryview_from_buffer", [](bool is_unsigned) { static const int16_t si16[] = { 3, 1, 4, 1, 5 }; @@ -432,8 +421,7 @@ TEST_SUBMODULE(pytypes, m) { m.def("pass_to_pybind11_bytes", [](py::bytes b) { return py::len(std::move(b)); }); m.def("pass_to_pybind11_str", [](py::str s) { return py::len(std::move(s)); }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("pass_to_std_string", [](std::string s) { return s.size(); }); + m.def("pass_to_std_string", [](const std::string &s) { return s.size(); }); // test_weakref m.def("weakref_from_handle", diff --git a/tests/test_stl.cpp b/tests/test_stl.cpp index dc75762e8..1f308e139 100644 --- a/tests/test_stl.cpp +++ b/tests/test_stl.cpp @@ -207,8 +207,7 @@ TEST_SUBMODULE(stl, m) { }, py::arg_v("x", std::nullopt, "None")); m.def("nodefer_none_optional", [](std::optional) { return true; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("nodefer_none_optional", [](py::none) { return false; }); + m.def("nodefer_none_optional", [](const py::none &) { return false; }); using opt_holder = OptionalHolder; py::class_(m, "OptionalHolder", "Class with optional member") @@ -299,12 +298,11 @@ TEST_SUBMODULE(stl, m) { m.def("stl_pass_by_pointer", [](std::vector* v) { return *v; }, "v"_a=nullptr); // #1258: pybind11/stl.h converts string to vector - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("func_with_string_or_vector_string_arg_overload", [](std::vector) { return 1; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("func_with_string_or_vector_string_arg_overload", [](std::list) { return 2; }); - // NOLINTNEXTLINE(performance-unnecessary-value-param) - m.def("func_with_string_or_vector_string_arg_overload", [](std::string) { return 3; }); + m.def("func_with_string_or_vector_string_arg_overload", + [](const std::vector &) { return 1; }); + m.def("func_with_string_or_vector_string_arg_overload", + [](const std::list &) { return 2; }); + m.def("func_with_string_or_vector_string_arg_overload", [](const std::string &) { return 3; }); class Placeholder { public: From 7472d37a93514dd22bf27da4123ff7b7f2a1545a Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 12 Jul 2021 13:39:06 -0700 Subject: [PATCH 3/5] Adding iostream.h thread-safety documentation. (#2995) * Adding iostream.h thread-safety documentation. * Restoring `TestThread` code with added `std::lock_guard`. * Updating new comments to reflect new information. * Fixing up `git rebase -X theirs` accidents. --- docs/advanced/pycpp/utilities.rst | 11 ++++++++ include/pybind11/iostream.h | 43 +++++++++++++++++-------------- tests/test_iostream.cpp | 14 +++++++++- 3 files changed, 48 insertions(+), 20 deletions(-) diff --git a/docs/advanced/pycpp/utilities.rst b/docs/advanced/pycpp/utilities.rst index c15051fb9..8b8e5196a 100644 --- a/docs/advanced/pycpp/utilities.rst +++ b/docs/advanced/pycpp/utilities.rst @@ -47,6 +47,17 @@ redirects output to the corresponding Python streams: call_noisy_func(); }); +.. warning:: + + The implementation in ``pybind11/iostream.h`` is NOT thread safe. Multiple + threads writing to a redirected ostream concurrently cause data races + and potentially buffer overflows. Therefore it is currrently a requirement + that all (possibly) concurrent redirected ostream writes are protected by + a mutex. #HelpAppreciated: Work on iostream.h thread safety. For more + background see the discussions under + `PR #2982 `_ and + `PR #2995 `_. + This method respects flushes on the output streams and will flush if needed when the scoped guard is destroyed. This allows the output to be redirected in real time, such as to a Jupyter notebook. The two arguments, the C++ stream and diff --git a/include/pybind11/iostream.h b/include/pybind11/iostream.h index 89d1813ba..c98067ad8 100644 --- a/include/pybind11/iostream.h +++ b/include/pybind11/iostream.h @@ -5,6 +5,16 @@ All rights reserved. Use of this source code is governed by a BSD-style license that can be found in the LICENSE file. + + WARNING: The implementation in this file is NOT thread safe. Multiple + threads writing to a redirected ostream concurrently cause data races + and potentially buffer overflows. Therefore it is currrently a requirement + that all (possibly) concurrent redirected ostream writes are protected by + a mutex. + #HelpAppreciated: Work on iostream.h thread safety. + For more background see the discussions under + https://github.com/pybind/pybind11/pull/2982 and + https://github.com/pybind/pybind11/pull/2995. */ #pragma once @@ -85,30 +95,25 @@ private: return remainder; } - // This function must be non-virtual to be called in a destructor. If the - // rare MSVC test failure shows up with this version, then this should be - // simplified to a fully qualified call. + // This function must be non-virtual to be called in a destructor. int _sync() { if (pbase() != pptr()) { // If buffer is not empty gil_scoped_acquire tmp; - // Placed inside gil_scoped_acquire as a mutex to avoid a race. - if (pbase() != pptr()) { // Check again under the lock - // This subtraction cannot be negative, so dropping the sign. - auto size = static_cast(pptr() - pbase()); - size_t remainder = utf8_remainder(); + // This subtraction cannot be negative, so dropping the sign. + auto size = static_cast(pptr() - pbase()); + size_t remainder = utf8_remainder(); - if (size > remainder) { - str line(pbase(), size - remainder); - pywrite(line); - pyflush(); - } - - // Copy the remainder at the end of the buffer to the beginning: - if (remainder > 0) - std::memmove(pbase(), pptr() - remainder, remainder); - setp(pbase(), epptr()); - pbump(static_cast(remainder)); + if (size > remainder) { + str line(pbase(), size - remainder); + pywrite(line); + pyflush(); } + + // Copy the remainder at the end of the buffer to the beginning: + if (remainder > 0) + std::memmove(pbase(), pptr() - remainder, remainder); + setp(pbase(), epptr()); + pbump(static_cast(remainder)); } return 0; } diff --git a/tests/test_iostream.cpp b/tests/test_iostream.cpp index 908788007..c620b5949 100644 --- a/tests/test_iostream.cpp +++ b/tests/test_iostream.cpp @@ -15,6 +15,8 @@ #include "pybind11_tests.h" #include #include +#include +#include #include void noisy_function(const std::string &msg, bool flush) { @@ -35,8 +37,18 @@ void noisy_funct_dual(const std::string &msg, const std::string &emsg) { struct TestThread { TestThread() : stop_{false} { auto thread_f = [this] { + static std::mutex cout_mutex; while (!stop_) { - std::cout << "x" << std::flush; + { + // #HelpAppreciated: Work on iostream.h thread safety. + // Without this lock, the clang ThreadSanitizer (tsan) reliably reports a + // data race, and this test is predictably flakey on Windows. + // For more background see the discussion under + // https://github.com/pybind/pybind11/pull/2982 and + // https://github.com/pybind/pybind11/pull/2995. + const std::lock_guard lock(cout_mutex); + std::cout << "x" << std::flush; + } std::this_thread::sleep_for(std::chrono::microseconds(50)); } }; t_ = new std::thread(std::move(thread_f)); From f0a65c899c7245f1dbaabad73330b4120ed6f2d4 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Mon, 12 Jul 2021 16:57:28 -0400 Subject: [PATCH 4/5] docs(fix): spelling mistake in recent commit --- docs/advanced/pycpp/utilities.rst | 2 +- include/pybind11/iostream.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/advanced/pycpp/utilities.rst b/docs/advanced/pycpp/utilities.rst index 8b8e5196a..30429e84d 100644 --- a/docs/advanced/pycpp/utilities.rst +++ b/docs/advanced/pycpp/utilities.rst @@ -51,7 +51,7 @@ redirects output to the corresponding Python streams: The implementation in ``pybind11/iostream.h`` is NOT thread safe. Multiple threads writing to a redirected ostream concurrently cause data races - and potentially buffer overflows. Therefore it is currrently a requirement + and potentially buffer overflows. Therefore it is currently a requirement that all (possibly) concurrent redirected ostream writes are protected by a mutex. #HelpAppreciated: Work on iostream.h thread safety. For more background see the discussions under diff --git a/include/pybind11/iostream.h b/include/pybind11/iostream.h index c98067ad8..e4d209585 100644 --- a/include/pybind11/iostream.h +++ b/include/pybind11/iostream.h @@ -8,7 +8,7 @@ WARNING: The implementation in this file is NOT thread safe. Multiple threads writing to a redirected ostream concurrently cause data races - and potentially buffer overflows. Therefore it is currrently a requirement + and potentially buffer overflows. Therefore it is currently a requirement that all (possibly) concurrent redirected ostream writes are protected by a mutex. #HelpAppreciated: Work on iostream.h thread safety. From 9f11951b5beee10c86eaeba56707da22d913bdbd Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 12 Jul 2021 14:00:48 -0700 Subject: [PATCH 5/5] Fixing spelling errors that went undetected because the pre-commit spell check was added after the CI for PR #2995 last ran. (#3103)