From 5b7d08ecb2c6f56e0b71f6142b131f111cf70904 Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Fri, 2 Dec 2022 14:09:09 -0800 Subject: [PATCH 1/8] Auto select return value policy for clif_automatic --- .../detail/smart_holder_type_casters.h | 19 +++- tests/test_return_value_policy_override.cpp | 96 +++++++++++++++++++ tests/test_return_value_policy_override.py | 28 ++++++ 3 files changed, 141 insertions(+), 2 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 7d351ac1b7..9f95e8c72b 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -630,7 +630,8 @@ struct smart_holder_type_caster : smart_holder_type_caster_load, static handle cast(T const &src, return_value_policy policy, handle parent) { // type_caster_base BEGIN if (policy == return_value_policy::automatic - || policy == return_value_policy::automatic_reference) { + || policy == return_value_policy::automatic_reference + || policy == return_value_policy::_clif_automatic) { policy = return_value_policy::copy; } return cast(&src, policy, parent); @@ -638,11 +639,21 @@ struct smart_holder_type_caster : smart_holder_type_caster_load, } static handle cast(T &src, return_value_policy policy, handle parent) { + if (policy == return_value_policy::_clif_automatic) { + if (std::is_move_constructible::value) { + policy = return_value_policy::move; + } else { + policy = return_value_policy::automatic; + } + } return cast(const_cast(src), policy, parent); // Mutbl2Const } static handle cast(T const *src, return_value_policy policy, handle parent) { auto st = type_caster_base::src_and_type(src); + if (policy == return_value_policy::_clif_automatic) { + policy = return_value_policy::copy; + } return cast_const_raw_ptr( // Originally type_caster_generic::cast. st.first, policy, @@ -653,6 +664,9 @@ struct smart_holder_type_caster : smart_holder_type_caster_load, } static handle cast(T *src, return_value_policy policy, handle parent) { + if (policy == return_value_policy::_clif_automatic) { + policy = return_value_policy::reference; + } return cast(const_cast(src), policy, parent); // Mutbl2Const } @@ -867,7 +881,8 @@ struct smart_holder_type_caster> : smart_holder_type_caste static handle cast(std::unique_ptr &&src, return_value_policy policy, handle parent) { if (policy != return_value_policy::automatic && policy != return_value_policy::reference_internal - && policy != return_value_policy::move) { + && policy != return_value_policy::move + && policy != return_value_policy::_clif_automatic) { // SMART_HOLDER_WIP: IMPROVABLE: Error message. throw cast_error("Invalid return_value_policy for unique_ptr."); } diff --git a/tests/test_return_value_policy_override.cpp b/tests/test_return_value_policy_override.cpp index 130f990555..cc5734a378 100644 --- a/tests/test_return_value_policy_override.cpp +++ b/tests/test_return_value_policy_override.cpp @@ -1,11 +1,69 @@ +#include + #include "pybind11_tests.h" namespace test_return_value_policy_override { struct some_type {}; +struct obj { + std::string mtxt; + obj(const std::string &mtxt_) : mtxt(mtxt_) {} + obj(const obj &other) { mtxt = other.mtxt + "_CpCtor"; } + obj(obj &&other) { mtxt = other.mtxt + "_MvCtor"; } + obj &operator=(const obj &other) { + mtxt = other.mtxt + "_CpCtor"; + return *this; + } + obj &operator=(obj &&other) { + mtxt = other.mtxt + "_MvCtor"; + return *this; + } +}; + +struct nocopy { + std::string mtxt; + nocopy(const std::string &mtxt_) : mtxt(mtxt_) {} + nocopy(nocopy &&other) { mtxt = other.mtxt + "_MvCtor"; } + nocopy &operator=(nocopy &&other) { + mtxt = other.mtxt + "_MvCtor"; + return *this; + } +}; + +obj *return_pointer() { + static obj value("pointer"); + return &value; +} + +const obj *return_const_pointer() { + static obj value("const_pointer"); + return &value; +} + +obj &return_reference() { + static obj value("reference"); + return value; +} + +const obj &return_const_reference() { + static obj value("const_reference"); + return value; +} + +std::shared_ptr return_shared_pointer() { return std::make_shared("shared_pointer"); } + +std::unique_ptr return_unique_pointer() { return std::make_unique("unique_pointer"); } + +nocopy &return_reference_nocopy() { + static nocopy value("reference_nocopy"); + return value; +} + } // namespace test_return_value_policy_override +using test_return_value_policy_override::nocopy; +using test_return_value_policy_override::obj; using test_return_value_policy_override::some_type; namespace pybind11 { @@ -51,6 +109,9 @@ struct type_caster : type_caster_base { } // namespace detail } // namespace pybind11 +PYBIND11_SMART_HOLDER_TYPE_CASTERS(obj) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(nocopy) + TEST_SUBMODULE(return_value_policy_override, m) { m.def("return_value_with_default_policy", []() { return some_type(); }); m.def( @@ -79,4 +140,39 @@ TEST_SUBMODULE(return_value_policy_override, m) { return &value; }, py::return_value_policy::_clif_automatic); + + py::classh(m, "object").def(py::init()).def_readonly("mtxt", &obj::mtxt); + m.def( + "return_object_value_with_policy_clif_automatic", + []() { return obj("value"); }, + py::return_value_policy::_clif_automatic); + // test_return_value_policy_override::return_pointer with default policy + // causes crash + m.def("return_object_pointer_with_policy_clif_automatic", + &test_return_value_policy_override::return_pointer, + py::return_value_policy::_clif_automatic); + // test_return_value_policy_override::return_const_pointer with default + // policy causes crash + m.def("return_object_const_pointer_with_policy_clif_automatic", + &test_return_value_policy_override::return_const_pointer, + py::return_value_policy::_clif_automatic); + m.def("return_object_reference_with_policy_clif_automatic", + &test_return_value_policy_override::return_reference, + py::return_value_policy::_clif_automatic); + m.def("return_object_const_reference_with_policy_clif_automatic", + &test_return_value_policy_override::return_const_reference, + py::return_value_policy::_clif_automatic); + m.def("return_object_unique_ptr_with_policy_clif_automatic", + &test_return_value_policy_override::return_unique_pointer, + py::return_value_policy::_clif_automatic); + m.def("return_object_shared_ptr_with_policy_clif_automatic", + &test_return_value_policy_override::return_shared_pointer, + py::return_value_policy::_clif_automatic); + + py::classh(m, "nocopy") + .def(py::init()) + .def_readonly("mtxt", &nocopy::mtxt); + m.def("return_nocopy_reference_with_policy_clif_automatic", + &test_return_value_policy_override::return_reference_nocopy, + py::return_value_policy::_clif_automatic); } diff --git a/tests/test_return_value_policy_override.py b/tests/test_return_value_policy_override.py index 36cb5bbf0b..37385b9cb1 100644 --- a/tests/test_return_value_policy_override.py +++ b/tests/test_return_value_policy_override.py @@ -1,3 +1,5 @@ +import pytest + from pybind11_tests import return_value_policy_override as m @@ -11,3 +13,29 @@ def test_return_pointer(): assert m.return_pointer_with_default_policy() == "automatic" assert m.return_pointer_with_policy_move() == "move" assert m.return_pointer_with_policy_clif_automatic() == "_clif_automatic" + + +@pytest.mark.parametrize( + "func, expected", + [ + (m.return_object_value_with_policy_clif_automatic, "value_MvCtor"), + (m.return_object_pointer_with_policy_clif_automatic, "pointer"), + ( + m.return_object_const_pointer_with_policy_clif_automatic, + "const_pointer_CpCtor", + ), + (m.return_object_reference_with_policy_clif_automatic, "reference_MvCtor"), + ( + m.return_object_const_reference_with_policy_clif_automatic, + "const_reference_CpCtor", + ), + (m.return_object_unique_ptr_with_policy_clif_automatic, "unique_pointer"), + (m.return_object_shared_ptr_with_policy_clif_automatic, "shared_pointer"), + ( + m.return_nocopy_reference_with_policy_clif_automatic, + "reference_nocopy_MvCtor", + ), + ], +) +def test_clif_automatic_return_value_policy_override(func, expected): + assert func().mtxt == expected From d1b36c9f0614ec336f9be5ddbcb006fa1d017518 Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Fri, 2 Dec 2022 14:24:29 -0800 Subject: [PATCH 2/8] Try fixing test failures --- tests/test_return_value_policy_override.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/test_return_value_policy_override.cpp b/tests/test_return_value_policy_override.cpp index cc5734a378..08741a3c2e 100644 --- a/tests/test_return_value_policy_override.cpp +++ b/tests/test_return_value_policy_override.cpp @@ -24,7 +24,9 @@ struct obj { struct nocopy { std::string mtxt; nocopy(const std::string &mtxt_) : mtxt(mtxt_) {} + nocopy(const nocopy &) = delete; nocopy(nocopy &&other) { mtxt = other.mtxt + "_MvCtor"; } + nocopy &operator=(const nocopy &) = delete; nocopy &operator=(nocopy &&other) { mtxt = other.mtxt + "_MvCtor"; return *this; @@ -51,9 +53,13 @@ const obj &return_const_reference() { return value; } -std::shared_ptr return_shared_pointer() { return std::make_shared("shared_pointer"); } +std::shared_ptr return_shared_pointer() { + return std::shared_ptr(new obj("shared_pointer")); +} -std::unique_ptr return_unique_pointer() { return std::make_unique("unique_pointer"); } +std::unique_ptr return_unique_pointer() { + return std::unique_ptr(new obj("unique_pointer")); +} nocopy &return_reference_nocopy() { static nocopy value("reference_nocopy"); From 5b7bd24273a21d427f1d614e7f0c8a8f47c4ae4a Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Mon, 5 Dec 2022 11:52:53 -0800 Subject: [PATCH 3/8] Add more tests. --- tests/test_return_value_policy_override.cpp | 236 ++++++++++++++++---- tests/test_return_value_policy_override.py | 39 ++-- 2 files changed, 214 insertions(+), 61 deletions(-) diff --git a/tests/test_return_value_policy_override.cpp b/tests/test_return_value_policy_override.cpp index 08741a3c2e..ceab0d7be4 100644 --- a/tests/test_return_value_policy_override.cpp +++ b/tests/test_return_value_policy_override.cpp @@ -6,71 +6,163 @@ namespace test_return_value_policy_override { struct some_type {}; -struct obj { +// cp = copyable, mv = movable, 1 = yes, 0 = no +struct type_cp1_mv1 { std::string mtxt; - obj(const std::string &mtxt_) : mtxt(mtxt_) {} - obj(const obj &other) { mtxt = other.mtxt + "_CpCtor"; } - obj(obj &&other) { mtxt = other.mtxt + "_MvCtor"; } - obj &operator=(const obj &other) { + type_cp1_mv1(const std::string &mtxt_) : mtxt(mtxt_) {} + type_cp1_mv1(const type_cp1_mv1 &other) { mtxt = other.mtxt + "_CpCtor"; } + type_cp1_mv1(type_cp1_mv1 &&other) { mtxt = other.mtxt + "_MvCtor"; } + type_cp1_mv1 &operator=(const type_cp1_mv1 &other) { mtxt = other.mtxt + "_CpCtor"; return *this; } - obj &operator=(obj &&other) { + type_cp1_mv1 &operator=(type_cp1_mv1 &&other) { mtxt = other.mtxt + "_MvCtor"; return *this; } }; -struct nocopy { +// nocopy +struct type_cp0_mv1 { std::string mtxt; - nocopy(const std::string &mtxt_) : mtxt(mtxt_) {} - nocopy(const nocopy &) = delete; - nocopy(nocopy &&other) { mtxt = other.mtxt + "_MvCtor"; } - nocopy &operator=(const nocopy &) = delete; - nocopy &operator=(nocopy &&other) { + type_cp0_mv1(const std::string &mtxt_) : mtxt(mtxt_) {} + type_cp0_mv1(const type_cp0_mv1 &) = delete; + type_cp0_mv1(type_cp0_mv1 &&other) { mtxt = other.mtxt + "_MvCtor"; } + type_cp0_mv1 &operator=(const type_cp0_mv1 &) = delete; + type_cp0_mv1 &operator=(type_cp0_mv1 &&other) { mtxt = other.mtxt + "_MvCtor"; return *this; } }; -obj *return_pointer() { - static obj value("pointer"); +// nomove +struct type_cp1_mv0 { + std::string mtxt; + type_cp1_mv0(const std::string &mtxt_) : mtxt(mtxt_) {} + type_cp1_mv0(const type_cp1_mv0 &other) { mtxt = other.mtxt + "_CpCtor"; } + type_cp1_mv0(type_cp1_mv0 &&other) = delete; + type_cp1_mv0 &operator=(const type_cp1_mv0 &other) { + mtxt = other.mtxt + "_CpCtor"; + return *this; + } + type_cp1_mv0 &operator=(type_cp1_mv0 &&other) = delete; +}; + +// nocopy_nomove +struct type_cp0_mv0 { + std::string mtxt; + type_cp0_mv0(const std::string &mtxt_) : mtxt(mtxt_) {} + type_cp0_mv0(const type_cp0_mv0 &) = delete; + type_cp0_mv0(type_cp0_mv0 &&other) = delete; + type_cp0_mv0 &operator=(const type_cp0_mv0 &other) = delete; + type_cp0_mv0 &operator=(type_cp0_mv0 &&other) = delete; +}; + +type_cp1_mv1 return_value() { return type_cp1_mv1{"value"}; } + +type_cp1_mv1 *return_pointer() { + static type_cp1_mv1 value("pointer"); return &value; } -const obj *return_const_pointer() { - static obj value("const_pointer"); +const type_cp1_mv1 *return_const_pointer() { + static type_cp1_mv1 value("const_pointer"); return &value; } -obj &return_reference() { - static obj value("reference"); +type_cp1_mv1 &return_reference() { + static type_cp1_mv1 value("reference"); + return value; +} + +const type_cp1_mv1 &return_const_reference() { + static type_cp1_mv1 value("const_reference"); return value; } -const obj &return_const_reference() { - static obj value("const_reference"); +std::shared_ptr return_shared_pointer() { + return std::make_shared("shared_pointer"); +} + +std::unique_ptr return_unique_pointer() { + return std::unique_ptr(new type_cp1_mv1("unique_pointer")); +} + +type_cp0_mv1 return_value_nocopy() { return type_cp0_mv1{"value_nocopy"}; } + +type_cp0_mv1 *return_pointer_nocopy() { + static type_cp0_mv1 value("pointer_nocopy"); + return &value; +} + +const type_cp0_mv1 *return_const_pointer_nocopy() { + static type_cp0_mv1 value("const_pointer_nocopy"); + return &value; +} + +type_cp0_mv1 &return_reference_nocopy() { + static type_cp0_mv1 value("reference_nocopy"); return value; } -std::shared_ptr return_shared_pointer() { - return std::shared_ptr(new obj("shared_pointer")); +std::shared_ptr return_shared_pointer_nocopy() { + return std::make_shared("shared_pointer_nocopy"); +} + +std::unique_ptr return_unique_pointer_nocopy() { + return std::unique_ptr(new type_cp0_mv1("unique_pointer_nocopy")); } -std::unique_ptr return_unique_pointer() { - return std::unique_ptr(new obj("unique_pointer")); +type_cp1_mv0 return_value_nomove() { return type_cp1_mv0{"value_nomove"}; } + +type_cp1_mv0 *return_pointer_nomove() { + static type_cp1_mv0 value("pointer_nomove"); + return &value; +} + +const type_cp1_mv0 *return_const_pointer_nomove() { + static type_cp1_mv0 value("const_pointer_nomove"); + return &value; +} + +type_cp1_mv0 &return_reference_nomove() { + static type_cp1_mv0 value("reference_nomove"); + return value; } -nocopy &return_reference_nocopy() { - static nocopy value("reference_nocopy"); +const type_cp1_mv0 &return_const_reference_nomove() { + static type_cp1_mv0 value("const_reference_nomove"); return value; } +std::shared_ptr return_shared_pointer_nomove() { + return std::make_shared("shared_pointer_nomove"); +} + +std::unique_ptr return_unique_pointer_nomove() { + return std::unique_ptr(new type_cp1_mv0("unique_pointer_nomove")); +} + +type_cp0_mv0 *return_pointer_nocopy_nomove() { + static type_cp0_mv0 value("pointer_nocopy_nomove"); + return &value; +} + +std::shared_ptr return_shared_pointer_nocopy_nomove() { + return std::make_shared("shared_pointer_nocopy_nomove"); +} + +std::unique_ptr return_unique_pointer_nocopy_nomove() { + return std::unique_ptr(new type_cp0_mv0("unique_pointer_nocopy_nomove")); +} + } // namespace test_return_value_policy_override -using test_return_value_policy_override::nocopy; -using test_return_value_policy_override::obj; using test_return_value_policy_override::some_type; +using test_return_value_policy_override::type_cp0_mv0; +using test_return_value_policy_override::type_cp0_mv1; +using test_return_value_policy_override::type_cp1_mv0; +using test_return_value_policy_override::type_cp1_mv1; namespace pybind11 { namespace detail { @@ -115,8 +207,10 @@ struct type_caster : type_caster_base { } // namespace detail } // namespace pybind11 -PYBIND11_SMART_HOLDER_TYPE_CASTERS(obj) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(nocopy) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(type_cp1_mv1) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(type_cp0_mv1) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(type_cp1_mv0) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(type_cp0_mv0) TEST_SUBMODULE(return_value_policy_override, m) { m.def("return_value_with_default_policy", []() { return some_type(); }); @@ -147,38 +241,92 @@ TEST_SUBMODULE(return_value_policy_override, m) { }, py::return_value_policy::_clif_automatic); - py::classh(m, "object").def(py::init()).def_readonly("mtxt", &obj::mtxt); - m.def( - "return_object_value_with_policy_clif_automatic", - []() { return obj("value"); }, - py::return_value_policy::_clif_automatic); + py::classh(m, "type_cp1_mv1") + .def(py::init()) + .def_readonly("mtxt", &type_cp1_mv1::mtxt); + m.def("return_value", + &test_return_value_policy_override::return_value, + py::return_value_policy::_clif_automatic); // test_return_value_policy_override::return_pointer with default policy // causes crash - m.def("return_object_pointer_with_policy_clif_automatic", + m.def("return_pointer", &test_return_value_policy_override::return_pointer, py::return_value_policy::_clif_automatic); // test_return_value_policy_override::return_const_pointer with default // policy causes crash - m.def("return_object_const_pointer_with_policy_clif_automatic", + m.def("return_const_pointer", &test_return_value_policy_override::return_const_pointer, py::return_value_policy::_clif_automatic); - m.def("return_object_reference_with_policy_clif_automatic", + m.def("return_reference", &test_return_value_policy_override::return_reference, py::return_value_policy::_clif_automatic); - m.def("return_object_const_reference_with_policy_clif_automatic", + m.def("return_const_reference", &test_return_value_policy_override::return_const_reference, py::return_value_policy::_clif_automatic); - m.def("return_object_unique_ptr_with_policy_clif_automatic", + m.def("return_unique_pointer", &test_return_value_policy_override::return_unique_pointer, py::return_value_policy::_clif_automatic); - m.def("return_object_shared_ptr_with_policy_clif_automatic", + m.def("return_shared_pointer", &test_return_value_policy_override::return_shared_pointer, py::return_value_policy::_clif_automatic); - py::classh(m, "nocopy") + py::classh(m, "type_cp0_mv1") .def(py::init()) - .def_readonly("mtxt", &nocopy::mtxt); - m.def("return_nocopy_reference_with_policy_clif_automatic", + .def_readonly("mtxt", &type_cp0_mv1::mtxt); + m.def("return_value_nocopy", + &test_return_value_policy_override::return_value_nocopy, + py::return_value_policy::_clif_automatic); + m.def("return_pointer_nocopy", + &test_return_value_policy_override::return_pointer_nocopy, + py::return_value_policy::_clif_automatic); + m.def("return_const_pointer_nocopy", + &test_return_value_policy_override::return_const_pointer_nocopy, + py::return_value_policy::_clif_automatic); + m.def("return_reference_nocopy", &test_return_value_policy_override::return_reference_nocopy, py::return_value_policy::_clif_automatic); + m.def("return_shared_pointer_nocopy", + &test_return_value_policy_override::return_shared_pointer_nocopy, + py::return_value_policy::_clif_automatic); + m.def("return_unique_pointer_nocopy", + &test_return_value_policy_override::return_unique_pointer_nocopy, + py::return_value_policy::_clif_automatic); + + py::classh(m, "type_cp1_mv0") + .def(py::init()) + .def_readonly("mtxt", &type_cp1_mv0::mtxt); + m.def("return_value_nomove", + &test_return_value_policy_override::return_value_nomove, + py::return_value_policy::_clif_automatic); + m.def("return_pointer_nomove", + &test_return_value_policy_override::return_pointer_nomove, + py::return_value_policy::_clif_automatic); + m.def("return_const_pointer_nomove", + &test_return_value_policy_override::return_const_pointer_nomove, + py::return_value_policy::_clif_automatic); + m.def("return_reference_nomove", + &test_return_value_policy_override::return_reference_nomove, + py::return_value_policy::_clif_automatic); + m.def("return_const_reference_nomove", + &test_return_value_policy_override::return_const_reference_nomove, + py::return_value_policy::_clif_automatic); + m.def("return_shared_pointer_nomove", + &test_return_value_policy_override::return_shared_pointer_nomove, + py::return_value_policy::_clif_automatic); + m.def("return_unique_pointer_nomove", + &test_return_value_policy_override::return_unique_pointer_nomove, + py::return_value_policy::_clif_automatic); + + py::classh(m, "type_cp0_mv0") + .def(py::init()) + .def_readonly("mtxt", &type_cp0_mv0::mtxt); + m.def("return_pointer_nocopy_nomove", + &test_return_value_policy_override::return_pointer_nocopy_nomove, + py::return_value_policy::_clif_automatic); + m.def("return_shared_pointer_nocopy_nomove", + &test_return_value_policy_override::return_shared_pointer_nocopy_nomove, + py::return_value_policy::_clif_automatic); + m.def("return_unique_pointer_nocopy_nomove", + &test_return_value_policy_override::return_unique_pointer_nocopy_nomove, + py::return_value_policy::_clif_automatic); } diff --git a/tests/test_return_value_policy_override.py b/tests/test_return_value_policy_override.py index 37385b9cb1..fcda42f077 100644 --- a/tests/test_return_value_policy_override.py +++ b/tests/test_return_value_policy_override.py @@ -18,23 +18,28 @@ def test_return_pointer(): @pytest.mark.parametrize( "func, expected", [ - (m.return_object_value_with_policy_clif_automatic, "value_MvCtor"), - (m.return_object_pointer_with_policy_clif_automatic, "pointer"), - ( - m.return_object_const_pointer_with_policy_clif_automatic, - "const_pointer_CpCtor", - ), - (m.return_object_reference_with_policy_clif_automatic, "reference_MvCtor"), - ( - m.return_object_const_reference_with_policy_clif_automatic, - "const_reference_CpCtor", - ), - (m.return_object_unique_ptr_with_policy_clif_automatic, "unique_pointer"), - (m.return_object_shared_ptr_with_policy_clif_automatic, "shared_pointer"), - ( - m.return_nocopy_reference_with_policy_clif_automatic, - "reference_nocopy_MvCtor", - ), + (m.return_value, "value_MvCtor"), + (m.return_pointer, "pointer"), + (m.return_const_pointer, "const_pointer_CpCtor"), + (m.return_reference, "reference_MvCtor"), + (m.return_const_reference, "const_reference_CpCtor"), + (m.return_unique_pointer, "unique_pointer"), + (m.return_shared_pointer, "shared_pointer"), + (m.return_value_nocopy, "value_nocopy_MvCtor"), + (m.return_pointer_nocopy, "pointer_nocopy"), + (m.return_reference_nocopy, "reference_nocopy_MvCtor"), + (m.return_unique_pointer_nocopy, "unique_pointer_nocopy"), + (m.return_shared_pointer_nocopy, "shared_pointer_nocopy"), + (m.return_value_nomove, "value_nomove_CpCtor"), + (m.return_pointer_nomove, "pointer_nomove"), + (m.return_const_pointer_nomove, "const_pointer_nomove_CpCtor"), + (m.return_reference_nomove, "reference_nomove_CpCtor"), + (m.return_const_reference_nomove, "const_reference_nomove_CpCtor"), + (m.return_unique_pointer_nomove, "unique_pointer_nomove"), + (m.return_shared_pointer_nomove, "shared_pointer_nomove"), + (m.return_pointer_nocopy_nomove, "pointer_nocopy_nomove"), + (m.return_unique_pointer_nocopy_nomove, "unique_pointer_nocopy_nomove"), + (m.return_shared_pointer_nocopy_nomove, "shared_pointer_nocopy_nomove"), ], ) def test_clif_automatic_return_value_policy_override(func, expected): From 8f992dd93c8378d30a3697b3323fdf9a0ad91f40 Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Mon, 5 Dec 2022 11:54:07 -0800 Subject: [PATCH 4/8] remove comments --- tests/test_return_value_policy_override.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/test_return_value_policy_override.cpp b/tests/test_return_value_policy_override.cpp index ceab0d7be4..2f3d218133 100644 --- a/tests/test_return_value_policy_override.cpp +++ b/tests/test_return_value_policy_override.cpp @@ -247,13 +247,9 @@ TEST_SUBMODULE(return_value_policy_override, m) { m.def("return_value", &test_return_value_policy_override::return_value, py::return_value_policy::_clif_automatic); - // test_return_value_policy_override::return_pointer with default policy - // causes crash m.def("return_pointer", &test_return_value_policy_override::return_pointer, py::return_value_policy::_clif_automatic); - // test_return_value_policy_override::return_const_pointer with default - // policy causes crash m.def("return_const_pointer", &test_return_value_policy_override::return_const_pointer, py::return_value_policy::_clif_automatic); From 853c556e86d4e2d7a12ab1200a6b9544b585c94f Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Mon, 5 Dec 2022 12:16:39 -0800 Subject: [PATCH 5/8] Fix test failures --- tests/test_return_value_policy_override.cpp | 5 ++++- tests/test_return_value_policy_override.py | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/test_return_value_policy_override.cpp b/tests/test_return_value_policy_override.cpp index 2f3d218133..7eeb089898 100644 --- a/tests/test_return_value_policy_override.cpp +++ b/tests/test_return_value_policy_override.cpp @@ -113,7 +113,10 @@ std::unique_ptr return_unique_pointer_nocopy() { return std::unique_ptr(new type_cp0_mv1("unique_pointer_nocopy")); } -type_cp1_mv0 return_value_nomove() { return type_cp1_mv0{"value_nomove"}; } +type_cp1_mv0 return_value_nomove() { + type_cp1_mv0 value("value_nomove"); + return const_cast(value); +} type_cp1_mv0 *return_pointer_nomove() { static type_cp1_mv0 value("pointer_nomove"); diff --git a/tests/test_return_value_policy_override.py b/tests/test_return_value_policy_override.py index fcda42f077..1d83c656e6 100644 --- a/tests/test_return_value_policy_override.py +++ b/tests/test_return_value_policy_override.py @@ -30,7 +30,7 @@ def test_return_pointer(): (m.return_reference_nocopy, "reference_nocopy_MvCtor"), (m.return_unique_pointer_nocopy, "unique_pointer_nocopy"), (m.return_shared_pointer_nocopy, "shared_pointer_nocopy"), - (m.return_value_nomove, "value_nomove_CpCtor"), + (m.return_value_nomove, "value_nomove_CpCtor_CpCtor"), (m.return_pointer_nomove, "pointer_nomove"), (m.return_const_pointer_nomove, "const_pointer_nomove_CpCtor"), (m.return_reference_nomove, "reference_nomove_CpCtor"), From 74a9e7a1949d23c6792688f35a7b145ee610543d Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Mon, 5 Dec 2022 12:24:05 -0800 Subject: [PATCH 6/8] Fix test failures --- tests/test_return_value_policy_override.cpp | 8 -------- tests/test_return_value_policy_override.py | 1 - 2 files changed, 9 deletions(-) diff --git a/tests/test_return_value_policy_override.cpp b/tests/test_return_value_policy_override.cpp index 7eeb089898..92a628bdc0 100644 --- a/tests/test_return_value_policy_override.cpp +++ b/tests/test_return_value_policy_override.cpp @@ -113,11 +113,6 @@ std::unique_ptr return_unique_pointer_nocopy() { return std::unique_ptr(new type_cp0_mv1("unique_pointer_nocopy")); } -type_cp1_mv0 return_value_nomove() { - type_cp1_mv0 value("value_nomove"); - return const_cast(value); -} - type_cp1_mv0 *return_pointer_nomove() { static type_cp1_mv0 value("pointer_nomove"); return &value; @@ -294,9 +289,6 @@ TEST_SUBMODULE(return_value_policy_override, m) { py::classh(m, "type_cp1_mv0") .def(py::init()) .def_readonly("mtxt", &type_cp1_mv0::mtxt); - m.def("return_value_nomove", - &test_return_value_policy_override::return_value_nomove, - py::return_value_policy::_clif_automatic); m.def("return_pointer_nomove", &test_return_value_policy_override::return_pointer_nomove, py::return_value_policy::_clif_automatic); diff --git a/tests/test_return_value_policy_override.py b/tests/test_return_value_policy_override.py index 1d83c656e6..85bdd5146c 100644 --- a/tests/test_return_value_policy_override.py +++ b/tests/test_return_value_policy_override.py @@ -30,7 +30,6 @@ def test_return_pointer(): (m.return_reference_nocopy, "reference_nocopy_MvCtor"), (m.return_unique_pointer_nocopy, "unique_pointer_nocopy"), (m.return_shared_pointer_nocopy, "shared_pointer_nocopy"), - (m.return_value_nomove, "value_nomove_CpCtor_CpCtor"), (m.return_pointer_nomove, "pointer_nomove"), (m.return_const_pointer_nomove, "const_pointer_nomove_CpCtor"), (m.return_reference_nomove, "reference_nomove_CpCtor"), From 5f310ab0bcf0dce8148a1b81a810ab47771fac18 Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Mon, 5 Dec 2022 13:29:17 -0800 Subject: [PATCH 7/8] Fix test failure for windows platform --- tests/test_return_value_policy_override.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/test_return_value_policy_override.py b/tests/test_return_value_policy_override.py index 85bdd5146c..e8e2e18139 100644 --- a/tests/test_return_value_policy_override.py +++ b/tests/test_return_value_policy_override.py @@ -1,3 +1,5 @@ +import re + import pytest from pybind11_tests import return_value_policy_override as m @@ -18,14 +20,14 @@ def test_return_pointer(): @pytest.mark.parametrize( "func, expected", [ - (m.return_value, "value_MvCtor"), + (m.return_value, "value(_MvCtor)*_MvCtor"), (m.return_pointer, "pointer"), (m.return_const_pointer, "const_pointer_CpCtor"), (m.return_reference, "reference_MvCtor"), (m.return_const_reference, "const_reference_CpCtor"), (m.return_unique_pointer, "unique_pointer"), (m.return_shared_pointer, "shared_pointer"), - (m.return_value_nocopy, "value_nocopy_MvCtor"), + (m.return_value_nocopy, "value_nocopy(_MvCtor)*_MvCtor"), (m.return_pointer_nocopy, "pointer_nocopy"), (m.return_reference_nocopy, "reference_nocopy_MvCtor"), (m.return_unique_pointer_nocopy, "unique_pointer_nocopy"), @@ -42,4 +44,4 @@ def test_return_pointer(): ], ) def test_clif_automatic_return_value_policy_override(func, expected): - assert func().mtxt == expected + assert re.match(expected, func().mtxt) From 2e3c2b7658d51c416a783c2d63c9232c5e5869a2 Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Mon, 5 Dec 2022 13:48:59 -0800 Subject: [PATCH 8/8] Fix clangtidy --- tests/test_return_value_policy_override.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/test_return_value_policy_override.cpp b/tests/test_return_value_policy_override.cpp index 92a628bdc0..5d6a51b7c4 100644 --- a/tests/test_return_value_policy_override.cpp +++ b/tests/test_return_value_policy_override.cpp @@ -9,14 +9,14 @@ struct some_type {}; // cp = copyable, mv = movable, 1 = yes, 0 = no struct type_cp1_mv1 { std::string mtxt; - type_cp1_mv1(const std::string &mtxt_) : mtxt(mtxt_) {} + explicit type_cp1_mv1(const std::string &mtxt_) : mtxt(mtxt_) {} type_cp1_mv1(const type_cp1_mv1 &other) { mtxt = other.mtxt + "_CpCtor"; } - type_cp1_mv1(type_cp1_mv1 &&other) { mtxt = other.mtxt + "_MvCtor"; } + type_cp1_mv1(type_cp1_mv1 &&other) noexcept { mtxt = other.mtxt + "_MvCtor"; } type_cp1_mv1 &operator=(const type_cp1_mv1 &other) { mtxt = other.mtxt + "_CpCtor"; return *this; } - type_cp1_mv1 &operator=(type_cp1_mv1 &&other) { + type_cp1_mv1 &operator=(type_cp1_mv1 &&other) noexcept { mtxt = other.mtxt + "_MvCtor"; return *this; } @@ -25,11 +25,11 @@ struct type_cp1_mv1 { // nocopy struct type_cp0_mv1 { std::string mtxt; - type_cp0_mv1(const std::string &mtxt_) : mtxt(mtxt_) {} + explicit type_cp0_mv1(const std::string &mtxt_) : mtxt(mtxt_) {} type_cp0_mv1(const type_cp0_mv1 &) = delete; - type_cp0_mv1(type_cp0_mv1 &&other) { mtxt = other.mtxt + "_MvCtor"; } + type_cp0_mv1(type_cp0_mv1 &&other) noexcept { mtxt = other.mtxt + "_MvCtor"; } type_cp0_mv1 &operator=(const type_cp0_mv1 &) = delete; - type_cp0_mv1 &operator=(type_cp0_mv1 &&other) { + type_cp0_mv1 &operator=(type_cp0_mv1 &&other) noexcept { mtxt = other.mtxt + "_MvCtor"; return *this; } @@ -38,7 +38,7 @@ struct type_cp0_mv1 { // nomove struct type_cp1_mv0 { std::string mtxt; - type_cp1_mv0(const std::string &mtxt_) : mtxt(mtxt_) {} + explicit type_cp1_mv0(const std::string &mtxt_) : mtxt(mtxt_) {} type_cp1_mv0(const type_cp1_mv0 &other) { mtxt = other.mtxt + "_CpCtor"; } type_cp1_mv0(type_cp1_mv0 &&other) = delete; type_cp1_mv0 &operator=(const type_cp1_mv0 &other) { @@ -51,7 +51,7 @@ struct type_cp1_mv0 { // nocopy_nomove struct type_cp0_mv0 { std::string mtxt; - type_cp0_mv0(const std::string &mtxt_) : mtxt(mtxt_) {} + explicit type_cp0_mv0(const std::string &mtxt_) : mtxt(mtxt_) {} type_cp0_mv0(const type_cp0_mv0 &) = delete; type_cp0_mv0(type_cp0_mv0 &&other) = delete; type_cp0_mv0 &operator=(const type_cp0_mv0 &other) = delete;