From b542f2709f9dc6e0b1f644d84e4cbf13ca9bf9e8 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Thu, 9 Sep 2021 11:12:21 -0400 Subject: [PATCH 1/6] Enable clang-tidy readability-const-return --- .clang-tidy | 3 ++- include/pybind11/cast.h | 8 ++++---- include/pybind11/detail/descr.h | 3 ++- include/pybind11/pybind11.h | 4 ++-- include/pybind11/pytypes.h | 6 +++--- tests/test_eigen.cpp | 2 ++ 6 files changed, 15 insertions(+), 11 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index cefffba1ea..cde57521fc 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -30,9 +30,10 @@ modernize-use-override, modernize-use-using, *performance*, readability-avoid-const-params-in-decls, +readability-const-return-type, readability-container-size-empty, -readability-else-after-return, readability-delete-null-pointer, +readability-else-after-return, readability-implicit-bool-conversion, readability-make-member-function-const, readability-misplaced-array-index, diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 79bf506d81..5625759a19 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1176,13 +1176,13 @@ class argument_loader { } template - enable_if_t::value, Return> call(Func &&f) && { - return std::move(*this).template call_impl(std::forward(f), indices{}, Guard{}); + enable_if_t::value, remove_cv_t> call(Func &&f) && { + return std::move(*this).template call_impl>(std::forward(f), indices{}, Guard{}); } template enable_if_t::value, void_type> call(Func &&f) && { - std::move(*this).template call_impl(std::forward(f), indices{}, Guard{}); + std::move(*this).template call_impl>(std::forward(f), indices{}, Guard{}); return void_type(); } @@ -1204,7 +1204,7 @@ class argument_loader { } template - Return call_impl(Func &&f, index_sequence, Guard &&) && { + remove_cv_t call_impl(Func &&f, index_sequence, Guard &&) && { return std::forward(f)(cast_op(std::move(std::get(argcasters)))...); } diff --git a/include/pybind11/detail/descr.h b/include/pybind11/detail/descr.h index 0acfc7db37..3d1063c7d6 100644 --- a/include/pybind11/detail/descr.h +++ b/include/pybind11/detail/descr.h @@ -75,7 +75,8 @@ constexpr enable_if_t _(const T1 &d, const T2 &) { return d; } template constexpr enable_if_t _(const T1 &, const T2 &d) { return d; } -template auto constexpr _() -> decltype(int_to_str::digits) { +template +auto constexpr _() -> remove_cv_t::digits)> { return int_to_str::digits; } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 990c213049..fc078b7cb6 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1995,13 +1995,13 @@ template ()).first), #endif typename... Extra> -iterator make_key_iterator(Iterator first, Sentinel last, Extra &&... extra) { +iterator make_key_iterator(Iterator first, Sentinel last, Extra &&...extra) { using state = detail::iterator_state; if (!detail::get_type_info(typeid(state), false)) { class_(handle(), "iterator", pybind11::module_local()) .def("__iter__", [](state &s) -> state& { return s; }) - .def("__next__", [](state &s) -> KeyType { + .def("__next__", [](state &s) -> detail::remove_cv_t { if (!s.first_or_done) ++s.it; else diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index fa50eb1e3a..5a77f18f19 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -770,7 +770,7 @@ class sequence_fast_readonly { protected: using iterator_category = std::random_access_iterator_tag; using value_type = handle; - using reference = const handle; + using reference = handle; using pointer = arrow_proxy; sequence_fast_readonly(handle obj, ssize_t n) : ptr(PySequence_Fast_ITEMS(obj.ptr()) + n) { } @@ -813,7 +813,7 @@ class dict_readonly { protected: using iterator_category = std::forward_iterator_tag; using value_type = std::pair; - using reference = const value_type; + using reference = value_type; using pointer = arrow_proxy; dict_readonly() = default; @@ -958,7 +958,7 @@ class iterator : public object { using iterator_category = std::input_iterator_tag; using difference_type = ssize_t; using value_type = handle; - using reference = const handle; + using reference = handle; using pointer = const handle *; PYBIND11_OBJECT_DEFAULT(iterator, object, PyIter_Check) diff --git a/tests/test_eigen.cpp b/tests/test_eigen.cpp index 651be0575f..0f9d3d0a1d 100644 --- a/tests/test_eigen.cpp +++ b/tests/test_eigen.cpp @@ -178,6 +178,7 @@ TEST_SUBMODULE(eigen, m) { ReturnTester() { print_created(this); } ~ReturnTester() { print_destroyed(this); } static Eigen::MatrixXd create() { return Eigen::MatrixXd::Ones(10, 10); } + // NOLINTNEXTLINE(readability-const-return-type) static const Eigen::MatrixXd createConst() { return Eigen::MatrixXd::Ones(10, 10); } Eigen::MatrixXd &get() { return mat; } Eigen::MatrixXd *getPtr() { return &mat; } @@ -244,6 +245,7 @@ TEST_SUBMODULE(eigen, m) { // test_fixed, and various other tests m.def("fixed_r", [mat]() -> FixedMatrixR { return FixedMatrixR(mat); }); + // NOLINTNEXTLINE(readability-const-return-type) m.def("fixed_r_const", [mat]() -> const FixedMatrixR { return FixedMatrixR(mat); }); m.def("fixed_c", [mat]() -> FixedMatrixC { return FixedMatrixC(mat); }); m.def("fixed_copy_r", [](const FixedMatrixR &m) -> FixedMatrixR { return m; }); From 63bf253ea8c17623b09f5575518b0392aec3cb22 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Thu, 9 Sep 2021 11:30:19 -0400 Subject: [PATCH 2/6] PyTest functional --- include/pybind11/cast.h | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 5d21768514..eab30eef7c 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -82,7 +82,7 @@ template class type_caster> { return caster_t::cast(&src.get(), policy, parent); } template using cast_op_type = std::reference_wrapper; - explicit operator std::reference_wrapper() { return cast_op(subcaster); } + operator std::reference_wrapper() { return cast_op(subcaster); } }; #define PYBIND11_TYPE_CASTER(type, py_name) \ @@ -279,7 +279,7 @@ template <> class type_caster : public type_caster { } template using cast_op_type = void*&; - explicit operator void *&() { return value; } + operator void *&() { return value; } static constexpr auto name = _("capsule"); private: void *value = nullptr; @@ -487,10 +487,8 @@ template struct type_caster(static_cast(str_caster).c_str()); - } - explicit operator CharT &() { + operator CharT*() { return none ? nullptr : const_cast(static_cast(str_caster).c_str()); } + operator CharT&() { if (none) throw value_error("Cannot convert None to a character"); @@ -583,8 +581,8 @@ template class Tuple, typename... Ts> class tuple_caster template using cast_op_type = type; - explicit operator type() & { return implicit_cast(indices{}); } - explicit operator type() && { return std::move(*this).implicit_cast(indices{}); } + operator type() & { return implicit_cast(indices{}); } + operator type() && { return std::move(*this).implicit_cast(indices{}); } protected: template @@ -1178,13 +1176,17 @@ class argument_loader { } template - enable_if_t::value, remove_cv_t> call(Func &&f) && { - return std::move(*this).template call_impl>(std::forward(f), indices{}, Guard{}); + // NOLINTNEXTLINE(readability-const-return-type) + enable_if_t::value, Return> call(Func &&f) && { + // NOLINTNEXTLINE(readability-const-return-type) + return std::move(*this).template call_impl(std::forward(f), indices{}, Guard{}); } template + // NOLINTNEXTLINE(readability-const-return-type) enable_if_t::value, void_type> call(Func &&f) && { - std::move(*this).template call_impl>(std::forward(f), indices{}, Guard{}); + // NOLINTNEXTLINE(readability-const-return-type) + std::move(*this).template call_impl(std::forward(f), indices{}, Guard{}); return void_type(); } @@ -1206,7 +1208,8 @@ class argument_loader { } template - remove_cv_t call_impl(Func &&f, index_sequence, Guard &&) && { + // NOLINTNEXTLINE(readability-const-return-type) + Return call_impl(Func &&f, index_sequence, Guard &&) && { return std::forward(f)(cast_op(std::move(std::get(argcasters)))...); } From 8fef23c4d1a1ea9ae81c50657dae80b5a432d39d Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Thu, 9 Sep 2021 11:44:06 -0400 Subject: [PATCH 3/6] Fix regression --- include/pybind11/cast.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index eab30eef7c..54ac743280 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1208,7 +1208,6 @@ class argument_loader { } template - // NOLINTNEXTLINE(readability-const-return-type) Return call_impl(Func &&f, index_sequence, Guard &&) && { return std::forward(f)(cast_op(std::move(std::get(argcasters)))...); } From 3a9516f7c3d1c30facb07c4dcc3c5cdaca82c64f Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Thu, 9 Sep 2021 11:52:27 -0400 Subject: [PATCH 4/6] Fix actual regression --- include/pybind11/cast.h | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 54ac743280..1d55f163f9 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -82,7 +82,7 @@ template class type_caster> { return caster_t::cast(&src.get(), policy, parent); } template using cast_op_type = std::reference_wrapper; - operator std::reference_wrapper() { return cast_op(subcaster); } + explicit operator std::reference_wrapper() { return cast_op(subcaster); } }; #define PYBIND11_TYPE_CASTER(type, py_name) \ @@ -279,7 +279,7 @@ template <> class type_caster : public type_caster { } template using cast_op_type = void*&; - operator void *&() { return value; } + explicit operator void *&() { return value; } static constexpr auto name = _("capsule"); private: void *value = nullptr; @@ -487,8 +487,10 @@ template struct type_caster(static_cast(str_caster).c_str()); } - operator CharT&() { + explicit operator CharT *() { + return none ? nullptr : const_cast(static_cast(str_caster).c_str()); + } + explicit operator CharT &() { if (none) throw value_error("Cannot convert None to a character"); @@ -581,8 +583,8 @@ template class Tuple, typename... Ts> class tuple_caster template using cast_op_type = type; - operator type() & { return implicit_cast(indices{}); } - operator type() && { return std::move(*this).implicit_cast(indices{}); } + explicit operator type() & { return implicit_cast(indices{}); } + explicit operator type() && { return std::move(*this).implicit_cast(indices{}); } protected: template @@ -1208,6 +1210,7 @@ class argument_loader { } template + // NOLINTNEXTLINE(readability-const-return-type) Return call_impl(Func &&f, index_sequence, Guard &&) && { return std::forward(f)(cast_op(std::move(std::get(argcasters)))...); } From 39409604c2ee556ebccf2156e7bc855425cf091b Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Thu, 9 Sep 2021 11:57:37 -0400 Subject: [PATCH 5/6] Remove one more NOLINT --- include/pybind11/cast.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 1d55f163f9..13ae3e9c1d 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1180,15 +1180,13 @@ class argument_loader { template // NOLINTNEXTLINE(readability-const-return-type) enable_if_t::value, Return> call(Func &&f) && { - // NOLINTNEXTLINE(readability-const-return-type) - return std::move(*this).template call_impl(std::forward(f), indices{}, Guard{}); + return std::move(*this).template call_impl>(std::forward(f), indices{}, Guard{}); } template // NOLINTNEXTLINE(readability-const-return-type) enable_if_t::value, void_type> call(Func &&f) && { - // NOLINTNEXTLINE(readability-const-return-type) - std::move(*this).template call_impl(std::forward(f), indices{}, Guard{}); + std::move(*this).template call_impl>(std::forward(f), indices{}, Guard{}); return void_type(); } From 2874d74e21614e0419ee0b0c4e1336312afd9f45 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Thu, 9 Sep 2021 13:37:42 -0400 Subject: [PATCH 6/6] Update comment --- tests/test_eigen.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_eigen.cpp b/tests/test_eigen.cpp index 0f9d3d0a1d..ad572b2ad7 100644 --- a/tests/test_eigen.cpp +++ b/tests/test_eigen.cpp @@ -245,6 +245,8 @@ TEST_SUBMODULE(eigen, m) { // test_fixed, and various other tests m.def("fixed_r", [mat]() -> FixedMatrixR { return FixedMatrixR(mat); }); + // Our Eigen does a hack which respects constness through the numpy writeable flag. + // Therefore, the const return actually affects this type despite being an rvalue. // NOLINTNEXTLINE(readability-const-return-type) m.def("fixed_r_const", [mat]() -> const FixedMatrixR { return FixedMatrixR(mat); }); m.def("fixed_c", [mat]() -> FixedMatrixC { return FixedMatrixC(mat); });