From 9a7f1a85dadaed1d9cc41deaa8a8a73a34a09689 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Fri, 19 Mar 2021 22:29:29 +0100 Subject: [PATCH 1/6] Test return by reference ... which shouldn't copy or move the argument --- tests/test_class_sh_basic.cpp | 2 +- tests/test_class_sh_basic.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_class_sh_basic.cpp b/tests/test_class_sh_basic.cpp index ff9f97607d..252c401f86 100644 --- a/tests/test_class_sh_basic.cpp +++ b/tests/test_class_sh_basic.cpp @@ -102,7 +102,7 @@ TEST_SUBMODULE(class_sh_basic, m) { m.def("rtrn_valu", rtrn_valu); m.def("rtrn_rref", rtrn_rref); m.def("rtrn_cref", rtrn_cref); - m.def("rtrn_mref", rtrn_mref); + m.def("rtrn_mref", rtrn_mref, py::return_value_policy::reference); m.def("rtrn_cptr", rtrn_cptr); m.def("rtrn_mptr", rtrn_mptr); diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index 3727dcdb59..f46688dcc1 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -22,7 +22,7 @@ def test_atyp_constructors(): (m.rtrn_valu, "rtrn_valu(_MvCtor)*_MvCtor"), (m.rtrn_rref, "rtrn_rref(_MvCtor)*_MvCtor"), (m.rtrn_cref, "rtrn_cref(_MvCtor)*_CpCtor"), - (m.rtrn_mref, "rtrn_mref(_MvCtor)*_CpCtor"), + (m.rtrn_mref, "rtrn_mref"), (m.rtrn_cptr, "rtrn_cptr"), (m.rtrn_mptr, "rtrn_mptr"), (m.rtrn_shmp, "rtrn_shmp"), From 1d1d72336de123e5c45f6e452dffb0b25164d9c1 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Sat, 20 Mar 2021 03:20:05 +0100 Subject: [PATCH 2/6] Add (failing) roundtrip test for const unique_ptr reference --- .../detail/smart_holder_type_casters.h | 3 + tests/test_class_sh_basic.cpp | 63 ++++++++------ tests/test_class_sh_basic.py | 85 ++++++++++++++----- 3 files changed, 102 insertions(+), 49 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 14b3ea9979..2733023ff4 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -731,6 +731,9 @@ struct smart_holder_type_caster> : smart_holder_type_caste template using cast_op_type = std::unique_ptr; + // TODO: This always returns a new, moving unique_ptr instance to the raw pointer, + // even if argument should be passed as reference. + // See test_class_sh_basic.py::test_unique_ptr_cref_roundtrip operator std::unique_ptr() { return this->template loaded_as_unique_ptr(); } }; diff --git a/tests/test_class_sh_basic.cpp b/tests/test_class_sh_basic.cpp index 252c401f86..c0bbdab143 100644 --- a/tests/test_class_sh_basic.cpp +++ b/tests/test_class_sh_basic.cpp @@ -21,11 +21,17 @@ struct uconsumer { // unique_ptr consumer std::unique_ptr held; bool valid() const { return static_cast(held); } - void pass_valu(std::unique_ptr obj) { held = std::move(obj); } - void pass_rref(std::unique_ptr &&obj) { held = std::move(obj); } - std::unique_ptr rtrn_valu() { return std::move(held); } - std::unique_ptr &rtrn_lref() { return held; } - const std::unique_ptr &rtrn_cref() { return held; } + std::string pass_uq_valu(std::unique_ptr obj) { held = std::move(obj); return held->mtxt; } + std::string pass_uq_rref(std::unique_ptr &&obj) { held = std::move(obj); return held->mtxt; } + std::string pass_uq_cref(const std::unique_ptr &obj) { return obj->mtxt; } + std::string pass_cptr(const atyp *obj) { return obj->mtxt; } + std::string pass_cref(const atyp &obj) { return obj.mtxt; } + + std::unique_ptr rtrn_uq_valu() { return std::move(held); } + std::unique_ptr& rtrn_uq_lref() { return held; } + const std::unique_ptr& rtrn_uq_cref() { return held; } + const atyp* rtrn_cptr() { return held.get(); } + const atyp& rtrn_cref() { return *held; } }; // clang-format off @@ -133,27 +139,32 @@ TEST_SUBMODULE(class_sh_basic, m) { py::classh(m, "uconsumer") .def(py::init<>()) .def("valid", &uconsumer::valid) - .def("pass_valu", &uconsumer::pass_valu) - .def("pass_rref", &uconsumer::pass_rref) - .def("rtrn_valu", &uconsumer::rtrn_valu) - .def("rtrn_lref", &uconsumer::rtrn_lref) - .def("rtrn_cref", &uconsumer::rtrn_cref); - - // Helpers for testing. - // These require selected functions above to work first, as indicated: - m.def("get_mtxt", get_mtxt); // pass_cref - m.def("get_ptr", get_ptr); // pass_cref - - m.def("unique_ptr_roundtrip", unique_ptr_roundtrip); // pass_uqmp, rtrn_uqmp - m.def("unique_ptr_cref_roundtrip", unique_ptr_cref_roundtrip); - - py::classh(m, "SharedPtrStash") - .def(py::init<>()) - .def("Add", &SharedPtrStash::Add, py::arg("obj")); - - m.def("py_type_handle_of_atyp", []() { - return py::type::handle_of(); // Exercises static_cast in this function. - }); + .def("pass_uq_valu", &uconsumer::pass_uq_valu) + .def("pass_uq_rref", &uconsumer::pass_uq_rref) + .def("pass_uq_cref", &uconsumer::pass_uq_cref) + .def("pass_cptr", &uconsumer::pass_cptr) + .def("pass_cref", &uconsumer::pass_cref) + .def("rtrn_uq_valu", &uconsumer::rtrn_uq_valu) + .def("rtrn_uq_lref", &uconsumer::rtrn_uq_lref) + .def("rtrn_uq_cref", &uconsumer::rtrn_uq_cref) + .def("rtrn_cptr", &uconsumer::rtrn_cptr, py::return_value_policy::reference_internal) + .def("rtrn_cref", &uconsumer::rtrn_cref, py::return_value_policy::reference_internal); + + // Helpers for testing. + // These require selected functions above to work first, as indicated: + m.def("get_mtxt", get_mtxt); // pass_cref + m.def("get_ptr", get_ptr); // pass_cref + + m.def("unique_ptr_roundtrip", unique_ptr_roundtrip); // pass_uqmp, rtrn_uqmp + m.def("unique_ptr_cref_roundtrip", unique_ptr_cref_roundtrip); + + py::classh(m, "SharedPtrStash") + .def(py::init<>()) + .def("Add", &SharedPtrStash::Add, py::arg("obj")); + + m.def("py_type_handle_of_atyp", []() { + return py::type::handle_of(); // Exercises static_cast in this function. + }); } } // namespace class_sh_basic diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index f46688dcc1..d98ae40e1c 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -118,46 +118,85 @@ def test_unique_ptr_roundtrip(num_round_trips=1000): id_orig = id_rtrn -# This currently fails, because a unique_ptr is always loaded by value -# due to pybind11/detail/smart_holder_type_casters.h:689 -# I think, we need to provide more cast operators. -@pytest.mark.skip -def test_unique_ptr_cref_roundtrip(num_round_trips=1000): - orig = m.atyp("passenger") - id_orig = id(orig) - mtxt_orig = m.get_mtxt(orig) - - recycled = m.unique_ptr_cref_roundtrip(orig) - assert m.get_mtxt(orig) == mtxt_orig - assert m.get_mtxt(recycled) == mtxt_orig - assert id(recycled) == id_orig - - @pytest.mark.parametrize( "pass_f, rtrn_f, moved_out, moved_in", [ - (m.uconsumer.pass_valu, m.uconsumer.rtrn_valu, True, True), - (m.uconsumer.pass_rref, m.uconsumer.rtrn_valu, True, True), - (m.uconsumer.pass_valu, m.uconsumer.rtrn_lref, True, False), - (m.uconsumer.pass_valu, m.uconsumer.rtrn_cref, True, False), + (m.uconsumer.pass_uq_valu, m.uconsumer.rtrn_uq_valu, True, True), + (m.uconsumer.pass_uq_rref, m.uconsumer.rtrn_uq_valu, True, True), + (m.uconsumer.pass_uq_valu, m.uconsumer.rtrn_uq_lref, True, False), + (m.uconsumer.pass_uq_valu, m.uconsumer.rtrn_uq_cref, True, False), ], ) def test_unique_ptr_consumer_roundtrip(pass_f, rtrn_f, moved_out, moved_in): c = m.uconsumer() - assert not c.valid() recycled = m.atyp("passenger") mtxt_orig = m.get_mtxt(recycled) + ptr_orig = m.get_ptr(recycled) assert re.match("passenger_(MvCtor){1,2}", mtxt_orig) - pass_f(c, recycled) - if moved_out: + pass_f(c, recycled) # pass object to C++ consumer c + if moved_out: # if moved (always), ensure it is flagged as disowned with pytest.raises(ValueError) as excinfo: m.get_mtxt(recycled) assert "Python instance was disowned" in str(excinfo.value) recycled = rtrn_f(c) - assert c.valid() != moved_in + assert c.valid() != moved_in # consumer gave up ownership? + assert m.get_ptr(recycled) == ptr_orig # underlying C++ object never changes + assert m.get_mtxt(recycled) == mtxt_orig # object was not moved or copied + + +@pytest.mark.parametrize( + "rtrn_f", + [m.uconsumer.rtrn_uq_cref, m.uconsumer.rtrn_cref, m.uconsumer.rtrn_cptr], +) +@pytest.mark.parametrize( + "pass_f", + [ + # This fails with: ValueError: Cannot disown non-owning holder (loaded_as_unique_ptr). + # + # smart_holder_type_caster_load::loaded_as_unique_ptr() attempts to pass + # the not-owned cref as a new unique_ptr, which would eventually destroy the object, + # and is thus (correctly) suppressed. + # To fix this, smart_holder would need to store the (original) unique_ptr reference, + # e.g. using a union of unique_ptr + shared_ptr. + pytest.param(m.uconsumer.pass_uq_cref, marks=pytest.mark.xfail), + m.uconsumer.pass_cptr, + m.uconsumer.pass_cref, + ], +) +def test_unique_ptr_cref_consumer_roundtrip(rtrn_f, pass_f): + c = m.uconsumer() + passenger = m.atyp("passenger") + mtxt_orig = m.get_mtxt(passenger) + ptr_orig = m.get_ptr(passenger) + + c.pass_uq_valu(passenger) # moves passenger to C++ (checked above) + + for _ in range(10): + cref = rtrn_f(c) # fetches const reference, should keep-alive parent c + assert pass_f(c, cref) == mtxt_orig + assert m.get_ptr(cref) == ptr_orig + + +# This fails with: ValueError: Missing value for wrapped C++ type: Python instance was disowned +# when accessing the orig object after passing it into m.unique_ptr_cref_roundtrip(). +# This is because smart_holder_type_caster_load::loaded_as_unique_ptr() always moves. +@pytest.mark.xfail +def test_unique_ptr_cref_roundtrip(): + orig = m.atyp("passenger") + id_orig = id(orig) + ptr_orig = m.get_ptr(orig) + mtxt_orig = m.get_mtxt(orig) + + recycled = m.unique_ptr_cref_roundtrip(orig) + # passing by reference shouldn't change pointer + assert m.get_ptr(orig) == ptr_orig + assert m.get_ptr(recycled) == ptr_orig + # nor apply any copy or move construction + assert m.get_mtxt(orig) == mtxt_orig assert m.get_mtxt(recycled) == mtxt_orig + assert id(recycled) == id_orig def test_py_type_handle_of_atyp(): From 612a5976d40ddea7ca9c73e6a9cbf391fc6aa616 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Sat, 20 Mar 2021 09:01:35 +0100 Subject: [PATCH 3/6] test_class_sh_basic.cpp: rename uconsumer -> consumer --- tests/test_class_sh_basic.cpp | 28 ++++++++++++++-------------- tests/test_class_sh_basic.py | 20 ++++++++++---------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/tests/test_class_sh_basic.cpp b/tests/test_class_sh_basic.cpp index c0bbdab143..8240b582ef 100644 --- a/tests/test_class_sh_basic.cpp +++ b/tests/test_class_sh_basic.cpp @@ -17,7 +17,7 @@ struct atyp { // Short for "any type". atyp(atyp &&other) { mtxt = other.mtxt + "_MvCtor"; } }; -struct uconsumer { // unique_ptr consumer +struct consumer { // unique_ptr consumer std::unique_ptr held; bool valid() const { return static_cast(held); } @@ -90,7 +90,7 @@ struct SharedPtrStash { } // namespace pybind11_tests PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_basic::atyp) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_basic::uconsumer) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_basic::consumer) PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_basic::SharedPtrStash) namespace pybind11_tests { @@ -136,19 +136,19 @@ TEST_SUBMODULE(class_sh_basic, m) { m.def("pass_udmp", pass_udmp); m.def("pass_udcp", pass_udcp); - py::classh(m, "uconsumer") + py::classh(m, "consumer") .def(py::init<>()) - .def("valid", &uconsumer::valid) - .def("pass_uq_valu", &uconsumer::pass_uq_valu) - .def("pass_uq_rref", &uconsumer::pass_uq_rref) - .def("pass_uq_cref", &uconsumer::pass_uq_cref) - .def("pass_cptr", &uconsumer::pass_cptr) - .def("pass_cref", &uconsumer::pass_cref) - .def("rtrn_uq_valu", &uconsumer::rtrn_uq_valu) - .def("rtrn_uq_lref", &uconsumer::rtrn_uq_lref) - .def("rtrn_uq_cref", &uconsumer::rtrn_uq_cref) - .def("rtrn_cptr", &uconsumer::rtrn_cptr, py::return_value_policy::reference_internal) - .def("rtrn_cref", &uconsumer::rtrn_cref, py::return_value_policy::reference_internal); + .def("valid", &consumer::valid) + .def("pass_uq_valu", &consumer::pass_uq_valu) + .def("pass_uq_rref", &consumer::pass_uq_rref) + .def("pass_uq_cref", &consumer::pass_uq_cref) + .def("pass_cptr", &consumer::pass_cptr) + .def("pass_cref", &consumer::pass_cref) + .def("rtrn_uq_valu", &consumer::rtrn_uq_valu) + .def("rtrn_uq_lref", &consumer::rtrn_uq_lref) + .def("rtrn_uq_cref", &consumer::rtrn_uq_cref) + .def("rtrn_cptr", &consumer::rtrn_cptr, py::return_value_policy::reference_internal) + .def("rtrn_cref", &consumer::rtrn_cref, py::return_value_policy::reference_internal); // Helpers for testing. // These require selected functions above to work first, as indicated: diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index d98ae40e1c..a4eba89538 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -121,14 +121,14 @@ def test_unique_ptr_roundtrip(num_round_trips=1000): @pytest.mark.parametrize( "pass_f, rtrn_f, moved_out, moved_in", [ - (m.uconsumer.pass_uq_valu, m.uconsumer.rtrn_uq_valu, True, True), - (m.uconsumer.pass_uq_rref, m.uconsumer.rtrn_uq_valu, True, True), - (m.uconsumer.pass_uq_valu, m.uconsumer.rtrn_uq_lref, True, False), - (m.uconsumer.pass_uq_valu, m.uconsumer.rtrn_uq_cref, True, False), + (m.consumer.pass_uq_valu, m.consumer.rtrn_uq_valu, True, True), + (m.consumer.pass_uq_rref, m.consumer.rtrn_uq_valu, True, True), + (m.consumer.pass_uq_valu, m.consumer.rtrn_uq_lref, True, False), + (m.consumer.pass_uq_valu, m.consumer.rtrn_uq_cref, True, False), ], ) def test_unique_ptr_consumer_roundtrip(pass_f, rtrn_f, moved_out, moved_in): - c = m.uconsumer() + c = m.consumer() recycled = m.atyp("passenger") mtxt_orig = m.get_mtxt(recycled) ptr_orig = m.get_ptr(recycled) @@ -148,7 +148,7 @@ def test_unique_ptr_consumer_roundtrip(pass_f, rtrn_f, moved_out, moved_in): @pytest.mark.parametrize( "rtrn_f", - [m.uconsumer.rtrn_uq_cref, m.uconsumer.rtrn_cref, m.uconsumer.rtrn_cptr], + [m.consumer.rtrn_uq_cref, m.consumer.rtrn_cref, m.consumer.rtrn_cptr], ) @pytest.mark.parametrize( "pass_f", @@ -160,13 +160,13 @@ def test_unique_ptr_consumer_roundtrip(pass_f, rtrn_f, moved_out, moved_in): # and is thus (correctly) suppressed. # To fix this, smart_holder would need to store the (original) unique_ptr reference, # e.g. using a union of unique_ptr + shared_ptr. - pytest.param(m.uconsumer.pass_uq_cref, marks=pytest.mark.xfail), - m.uconsumer.pass_cptr, - m.uconsumer.pass_cref, + pytest.param(m.consumer.pass_uq_cref, marks=pytest.mark.xfail), + m.consumer.pass_cptr, + m.consumer.pass_cref, ], ) def test_unique_ptr_cref_consumer_roundtrip(rtrn_f, pass_f): - c = m.uconsumer() + c = m.consumer() passenger = m.atyp("passenger") mtxt_orig = m.get_mtxt(passenger) ptr_orig = m.get_ptr(passenger) From 8119419bf3e9e1432a95d6486bbc83b3e08c8d3d Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Sat, 20 Mar 2021 18:18:21 +0100 Subject: [PATCH 4/6] roundtrip test via reference passed to aliased class method Probably the test is failing, because it passes the arguments by value instead of by reference. --- tests/test_class_sh_basic.cpp | 47 ++++++++------- tests/test_class_sh_with_alias.cpp | 79 +++++++++++++++++++++++++ tests/test_class_sh_with_alias.py | 94 ++++++++++++++++++++++++++++++ 3 files changed, 200 insertions(+), 20 deletions(-) diff --git a/tests/test_class_sh_basic.cpp b/tests/test_class_sh_basic.cpp index 8240b582ef..562593ff48 100644 --- a/tests/test_class_sh_basic.cpp +++ b/tests/test_class_sh_basic.cpp @@ -2,6 +2,7 @@ #include +#include #include #include #include @@ -21,17 +22,23 @@ struct consumer { // unique_ptr consumer std::unique_ptr held; bool valid() const { return static_cast(held); } - std::string pass_uq_valu(std::unique_ptr obj) { held = std::move(obj); return held->mtxt; } - std::string pass_uq_rref(std::unique_ptr &&obj) { held = std::move(obj); return held->mtxt; } + std::string pass_uq_valu(std::unique_ptr obj) { + held = std::move(obj); + return held->mtxt; + } + std::string pass_uq_rref(std::unique_ptr &&obj) { + held = std::move(obj); + return held->mtxt; + } std::string pass_uq_cref(const std::unique_ptr &obj) { return obj->mtxt; } std::string pass_cptr(const atyp *obj) { return obj->mtxt; } std::string pass_cref(const atyp &obj) { return obj.mtxt; } - std::unique_ptr rtrn_uq_valu() { return std::move(held); } - std::unique_ptr& rtrn_uq_lref() { return held; } - const std::unique_ptr& rtrn_uq_cref() { return held; } - const atyp* rtrn_cptr() { return held.get(); } - const atyp& rtrn_cref() { return *held; } + std::unique_ptr rtrn_uq_valu() { return std::move(held); } + std::unique_ptr &rtrn_uq_lref() { return held; } + const std::unique_ptr &rtrn_uq_cref() { return held; } + const atyp *rtrn_cptr() { return held.get(); } + const atyp &rtrn_cref() { return *held; } }; // clang-format off @@ -74,7 +81,7 @@ std::string pass_udcp(std::unique_ptr obj) { return "pass_udcp // Helpers for testing. std::string get_mtxt(atyp const &obj) { return obj.mtxt; } -std::ptrdiff_t get_ptr(atyp const &obj) { return reinterpret_cast(&obj); } +std::uintptr_t get_ptr(atyp const &obj) { return reinterpret_cast(&obj); } std::unique_ptr unique_ptr_roundtrip(std::unique_ptr obj) { return obj; } const std::unique_ptr &unique_ptr_cref_roundtrip(const std::unique_ptr &obj) { @@ -150,21 +157,21 @@ TEST_SUBMODULE(class_sh_basic, m) { .def("rtrn_cptr", &consumer::rtrn_cptr, py::return_value_policy::reference_internal) .def("rtrn_cref", &consumer::rtrn_cref, py::return_value_policy::reference_internal); - // Helpers for testing. - // These require selected functions above to work first, as indicated: - m.def("get_mtxt", get_mtxt); // pass_cref - m.def("get_ptr", get_ptr); // pass_cref + // Helpers for testing. + // These require selected functions above to work first, as indicated: + m.def("get_mtxt", get_mtxt); // pass_cref + m.def("get_ptr", get_ptr); // pass_cref - m.def("unique_ptr_roundtrip", unique_ptr_roundtrip); // pass_uqmp, rtrn_uqmp - m.def("unique_ptr_cref_roundtrip", unique_ptr_cref_roundtrip); + m.def("unique_ptr_roundtrip", unique_ptr_roundtrip); // pass_uqmp, rtrn_uqmp + m.def("unique_ptr_cref_roundtrip", unique_ptr_cref_roundtrip); - py::classh(m, "SharedPtrStash") - .def(py::init<>()) - .def("Add", &SharedPtrStash::Add, py::arg("obj")); + py::classh(m, "SharedPtrStash") + .def(py::init<>()) + .def("Add", &SharedPtrStash::Add, py::arg("obj")); - m.def("py_type_handle_of_atyp", []() { - return py::type::handle_of(); // Exercises static_cast in this function. - }); + m.def("py_type_handle_of_atyp", []() { + return py::type::handle_of(); // Exercises static_cast in this function. + }); } } // namespace class_sh_basic diff --git a/tests/test_class_sh_with_alias.cpp b/tests/test_class_sh_with_alias.cpp index 846533ddfc..ad30a20b10 100644 --- a/tests/test_class_sh_with_alias.cpp +++ b/tests/test_class_sh_with_alias.cpp @@ -2,6 +2,7 @@ #include +#include #include namespace pybind11_tests { @@ -73,14 +74,92 @@ void wrap(py::module_ m, const char *py_class_name) { m.def("AddInCppUniquePtr", AddInCppUniquePtr, py::arg("obj"), py::arg("other_val")); } +struct Passenger { + std::string mtxt; + // on construction: store pointer as an id + Passenger() : mtxt(id() + "_") {} + Passenger(const Passenger &other) { mtxt = other.mtxt + "Copy->" + id(); } + Passenger(Passenger &&other) { mtxt = other.mtxt + "Move->" + id(); } + std::string id() const { return std::to_string(reinterpret_cast(this)); } +}; +struct ConsumerBase { + ConsumerBase() = default; + ConsumerBase(const ConsumerBase &) = default; + ConsumerBase(ConsumerBase &&) = default; + virtual ~ConsumerBase() = default; + virtual void pass_uq_cref(const std::unique_ptr &obj) { modify(*obj); }; + virtual void pass_valu(Passenger obj) { modify(obj); }; + virtual void pass_lref(Passenger &obj) { modify(obj); }; + virtual void pass_cref(const Passenger &obj) { modify(const_cast(obj)); }; + void modify(Passenger &obj) { + // when base virtual method is called: append obj pointer again (should be same as before) + obj.mtxt.append("_"); + obj.mtxt.append(std::to_string(reinterpret_cast(&obj))); + } +}; +struct ConsumerBaseAlias : ConsumerBase { + using ConsumerBase::ConsumerBase; + void pass_uq_cref(const std::unique_ptr &obj) override { + PYBIND11_OVERRIDE(void, ConsumerBase, pass_uq_cref, obj); + } + void pass_valu(Passenger obj) override { + PYBIND11_OVERRIDE(void, ConsumerBase, pass_valu, obj); + } + void pass_lref(Passenger &obj) override { + PYBIND11_OVERRIDE(void, ConsumerBase, pass_lref, obj); + } + void pass_cref(const Passenger &obj) override { + PYBIND11_OVERRIDE(void, ConsumerBase, pass_cref, obj); + } +}; + +// check roundtrip of Passenger send to ConsumerBaseAlias +// TODO: Find template magic to avoid code duplication +std::string check_roundtrip_uq_cref(ConsumerBase &consumer) { + std::unique_ptr obj(new Passenger()); + consumer.pass_uq_cref(obj); + return obj->mtxt; +} +std::string check_roundtrip_valu(ConsumerBase &consumer) { + Passenger obj; + consumer.pass_valu(obj); + return obj.mtxt; +} +std::string check_roundtrip_lref(ConsumerBase &consumer) { + Passenger obj; + consumer.pass_lref(obj); + return obj.mtxt; +} +std::string check_roundtrip_cref(ConsumerBase &consumer) { + Passenger obj; + consumer.pass_cref(obj); + return obj.mtxt; +} + } // namespace test_class_sh_with_alias } // namespace pybind11_tests PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::test_class_sh_with_alias::Abase<0>) PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::test_class_sh_with_alias::Abase<1>) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::test_class_sh_with_alias::Passenger) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::test_class_sh_with_alias::ConsumerBase) TEST_SUBMODULE(class_sh_with_alias, m) { using namespace pybind11_tests::test_class_sh_with_alias; wrap<0>(m, "Abase0"); wrap<1>(m, "Abase1"); + + py::classh(m, "Passenger").def_readwrite("mtxt", &Passenger::mtxt); + + py::classh(m, "ConsumerBase") + .def(py::init<>()) + .def("pass_uq_cref", &ConsumerBase::pass_uq_cref) + .def("pass_valu", &ConsumerBase::pass_valu) + .def("pass_lref", &ConsumerBase::pass_lref) + .def("pass_cref", &ConsumerBase::pass_cref); + + m.def("check_roundtrip_uq_cref", check_roundtrip_uq_cref); + m.def("check_roundtrip_valu", check_roundtrip_valu); + m.def("check_roundtrip_lref", check_roundtrip_lref); + m.def("check_roundtrip_cref", check_roundtrip_cref); } diff --git a/tests/test_class_sh_with_alias.py b/tests/test_class_sh_with_alias.py index 4650eaa5e1..0d6cd1e461 100644 --- a/tests/test_class_sh_with_alias.py +++ b/tests/test_class_sh_with_alias.py @@ -1,9 +1,17 @@ # -*- coding: utf-8 -*- +import re import pytest +import env # noqa: F401 from pybind11_tests import class_sh_with_alias as m +def check_regex(expected, actual): + result = re.match(expected + "$", actual) + if result is None: + pytest.fail("expected: '{}' != actual: '{}'".format(expected, actual)) + + class PyDrvd0(m.Abase0): def __init__(self, val): super(PyDrvd0, self).__init__(val) @@ -56,3 +64,89 @@ def test_drvd1_add_in_cpp_unique_ptr(): drvd = PyDrvd1(25) assert m.AddInCppUniquePtr(drvd, 83) == ((25 * 10 + 3) * 200 + 83) * 100 + 13 return # Comment out for manual leak checking (use `top` command). + + +class PyConsumer1(m.ConsumerBase): + def __init__(self): + m.ConsumerBase.__init__(self) + + def pass_uq_cref(self, obj): + obj.mtxt = obj.mtxt + "pass_uq_cref" + + def pass_valu(self, obj): + obj.mtxt = obj.mtxt + "pass_valu" + + def pass_lref(self, obj): + obj.mtxt = obj.mtxt + "pass_lref" + + def pass_cref(self, obj): + obj.mtxt = obj.mtxt + "pass_cref" + + +class PyConsumer2(m.ConsumerBase): + """This one, additionally to PyConsumer1 calls the base methods. + This results in a second call to the trampoline override dispatcher. + Hence arguments have travelled a long way back and forth between C++ + and Python: C++ -> Python (call #1) -> C++ (call #2).""" + + def __init__(self): + m.ConsumerBase.__init__(self) + + def pass_uq_cref(self, obj): + obj.mtxt = obj.mtxt + "pass_uq_cref" + m.ConsumerBase.pass_uq_cref(self, obj) + + def pass_valu(self, obj): + obj.mtxt = obj.mtxt + "pass_valu" + m.ConsumerBase.pass_valu(self, obj) + + def pass_lref(self, obj): + obj.mtxt = obj.mtxt + "pass_lref" + m.ConsumerBase.pass_lref(self, obj) + + def pass_cref(self, obj): + obj.mtxt = obj.mtxt + "pass_cref" + m.ConsumerBase.pass_cref(self, obj) + + +# roundtrip tests, creating an object in C++ that is passed by reference +# to a virtual method of a class derived in Python. Thus: +# C++ -> Python -> C++ +@pytest.mark.parametrize( + "f, expected", + [ + (m.check_roundtrip_uq_cref, "([0-9]+)_pass_uq_cref"), + (m.check_roundtrip_valu, "([0-9]+)_"), # modification not passed back to C++ + (m.check_roundtrip_lref, "([0-9]+)_pass_lref"), + pytest.param( + m.check_roundtrip_cref, + "([0-9]+)_pass_cref", + marks=pytest.mark.skipif("env.PYPY"), + ), + ], +) +def test_unique_ptr_consumer1_roundtrip(f, expected): + c = PyConsumer1() + check_regex(expected, f(c)) + + +@pytest.mark.parametrize( + "f, expected", + [ + pytest.param( # cannot (yet) load unowned const unique_ptr& (for 2nd call) + m.check_roundtrip_uq_cref, + "([0-9]+)_pass_uq_cref_\\1", + marks=pytest.mark.xfail, + ), + (m.check_roundtrip_valu, "([0-9]+)_"), # modification not passed back to C++ + (m.check_roundtrip_lref, "([0-9]+)_pass_lref_\\1"), + pytest.param( # PYPY always copies the argument instead of passing the reference + m.check_roundtrip_cref, + "([0-9]+)_pass_cref_\\1", + marks=pytest.mark.skipif("env.PYPY"), + ), + ], +) +def test_unique_ptr_consumer2_roundtrip(f, expected): + c = PyConsumer2() + check_regex(expected, f(c)) From 4b0b8ae559232f6ccfd13b8bf7a6f4ac12bc19dd Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Sun, 21 Mar 2021 03:31:53 +0100 Subject: [PATCH 5/6] Fix roundtrip unit test for derived Python objects As expected, the unit test was failing because args were passed by-value and not by reference. However, it wasn't possible to just change the policy to reference or reference_internal. reference gave errors, because const unique_ptr& only accepts reference_internal. reference_internal gives an error, because the parent required for keep_alive is null. Hence, I decided to introduce a new policy to explicitly indicate passing from from Python to C++ via an overridden method. That's the cleanest solution to correctly react to the special needs of this use case. --- include/pybind11/detail/common.h | 6 +++++- .../detail/smart_holder_type_casters.h | 20 +++++++++++-------- include/pybind11/detail/type_caster_base.h | 2 ++ include/pybind11/pytypes.h | 4 ++-- 4 files changed, 21 insertions(+), 11 deletions(-) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 17021978a1..272d0c1c76 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -434,7 +434,11 @@ enum class return_value_policy : uint8_t { collected while Python is still using the child. More advanced variations of this scheme are also possible using combinations of return_value_policy::reference and the keep_alive call policy */ - reference_internal + reference_internal, + + /* This internally-only used policy applies to C++ arguments passed + to virtual methods overridden in Python to allow reference passing. */ + automatic_override }; PYBIND11_NAMESPACE_BEGIN(detail) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 2733023ff4..509020ccc8 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -472,6 +472,10 @@ struct smart_holder_type_caster : smart_holder_type_caster_load, } static handle cast(T const &src, return_value_policy policy, handle parent) { + return cast(const_cast(src), policy, parent); + } + + static handle cast(T &src, return_value_policy policy, handle parent) { // type_caster_base BEGIN // clang-format off if (policy == return_value_policy::automatic || policy == return_value_policy::automatic_reference) @@ -481,11 +485,13 @@ struct smart_holder_type_caster : smart_holder_type_caster_load, // type_caster_base END } - static handle cast(T &src, return_value_policy policy, handle parent) { - return cast(const_cast(src), policy, parent); // Mutbl2Const + static handle cast(T const *src, return_value_policy policy, handle parent) { + return cast(const_cast(src), policy, parent); } - static handle cast(T const *src, return_value_policy policy, handle parent) { + static handle cast(T *src, return_value_policy policy, handle parent) { + if (policy == return_value_policy::automatic_override) + policy = return_value_policy::reference; auto st = type_caster_base::src_and_type(src); return cast_const_raw_ptr( // Originally type_caster_generic::cast. st.first, @@ -496,10 +502,6 @@ struct smart_holder_type_caster : smart_holder_type_caster_load, make_constructor::make_move_constructor(src)); } - static handle cast(T *src, return_value_policy policy, handle parent) { - return cast(const_cast(src), policy, parent); // Mutbl2Const - } - #if defined(_MSC_VER) && _MSC_VER < 1910 // Working around MSVC 2015 bug. const-correctness is lost. // SMART_HOLDER_WIP: IMPROVABLE: make common code work with MSVC 2015. @@ -723,7 +725,9 @@ struct smart_holder_type_caster> : smart_holder_type_caste return none().release(); if (policy == return_value_policy::automatic) policy = return_value_policy::reference_internal; - if (policy != return_value_policy::reference_internal) + else if (policy == return_value_policy::automatic_override) + ; + else if (policy != return_value_policy::reference_internal) throw cast_error("Invalid return_value_policy for unique_ptr&"); return smart_holder_type_caster::cast(src.get(), policy, parent); } diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 8ebe39879a..76cfae59d2 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -899,6 +899,8 @@ template class type_caster_base : public type_caster_generic { } static handle cast(const itype *src, return_value_policy policy, handle parent) { + if (policy == return_value_policy::automatic_override) + policy = return_value_policy::reference; auto st = src_and_type(src); return type_caster_generic::cast( st.first, policy, parent, st.second, diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 6f0e6067ef..0303f263ba 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -105,9 +105,9 @@ class object_api : public pyobject_tag { function will throw a `cast_error` exception. When the Python function call fails, a `error_already_set` exception is thrown. \endrst */ - template + template object operator()(Args &&...args) const; - template + template PYBIND11_DEPRECATED("call(...) was deprecated in favor of operator()(...)") object call(Args&&... args) const; From 549062481aeb99c886f79c8fe29f0af35eddbc15 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Sun, 21 Mar 2021 03:43:59 +0100 Subject: [PATCH 6/6] Improve accuaracy of unit tests: Count num of copies/moves --- tests/test_class_sh_basic.py | 38 +++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index a4eba89538..377c61af55 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -16,12 +16,18 @@ def test_atyp_constructors(): assert obj.__class__.__name__ == "atyp" +def check_regex(expected, actual): + result = re.match(expected + "$", actual) + if result is None: + pytest.fail("expected: '{}' != actual: '{}'".format(expected, actual)) + + @pytest.mark.parametrize( "rtrn_f, expected", [ - (m.rtrn_valu, "rtrn_valu(_MvCtor)*_MvCtor"), - (m.rtrn_rref, "rtrn_rref(_MvCtor)*_MvCtor"), - (m.rtrn_cref, "rtrn_cref(_MvCtor)*_CpCtor"), + (m.rtrn_valu, "rtrn_valu(_MvCtor){1,3}"), + (m.rtrn_rref, "rtrn_rref(_MvCtor){1}"), + (m.rtrn_cref, "rtrn_cref_CpCtor"), (m.rtrn_mref, "rtrn_mref"), (m.rtrn_cptr, "rtrn_cptr"), (m.rtrn_mptr, "rtrn_mptr"), @@ -34,25 +40,25 @@ def test_atyp_constructors(): ], ) def test_cast(rtrn_f, expected): - assert re.match(expected, m.get_mtxt(rtrn_f())) + check_regex(expected, m.get_mtxt(rtrn_f())) @pytest.mark.parametrize( "pass_f, mtxt, expected", [ - (m.pass_valu, "Valu", "pass_valu:Valu(_MvCtor)*_CpCtor"), - (m.pass_cref, "Cref", "pass_cref:Cref(_MvCtor)*_MvCtor"), - (m.pass_mref, "Mref", "pass_mref:Mref(_MvCtor)*_MvCtor"), - (m.pass_cptr, "Cptr", "pass_cptr:Cptr(_MvCtor)*_MvCtor"), - (m.pass_mptr, "Mptr", "pass_mptr:Mptr(_MvCtor)*_MvCtor"), - (m.pass_shmp, "Shmp", "pass_shmp:Shmp(_MvCtor)*_MvCtor"), - (m.pass_shcp, "Shcp", "pass_shcp:Shcp(_MvCtor)*_MvCtor"), - (m.pass_uqmp, "Uqmp", "pass_uqmp:Uqmp(_MvCtor)*_MvCtor"), - (m.pass_uqcp, "Uqcp", "pass_uqcp:Uqcp(_MvCtor)*_MvCtor"), + (m.pass_valu, "Valu", "pass_valu:Valu(_MvCtor){1,2}_CpCtor"), + (m.pass_cref, "Cref", "pass_cref:Cref(_MvCtor){1,2}"), + (m.pass_mref, "Mref", "pass_mref:Mref(_MvCtor){1,2}"), + (m.pass_cptr, "Cptr", "pass_cptr:Cptr(_MvCtor){1,2}"), + (m.pass_mptr, "Mptr", "pass_mptr:Mptr(_MvCtor){1,2}"), + (m.pass_shmp, "Shmp", "pass_shmp:Shmp(_MvCtor){1,2}"), + (m.pass_shcp, "Shcp", "pass_shcp:Shcp(_MvCtor){1,2}"), + (m.pass_uqmp, "Uqmp", "pass_uqmp:Uqmp(_MvCtor){1,2}"), + (m.pass_uqcp, "Uqcp", "pass_uqcp:Uqcp(_MvCtor){1,2}"), ], ) def test_load_with_mtxt(pass_f, mtxt, expected): - assert re.match(expected, pass_f(m.atyp(mtxt))) + check_regex(expected, pass_f(m.atyp(mtxt))) @pytest.mark.parametrize( @@ -111,7 +117,7 @@ def test_unique_ptr_roundtrip(num_round_trips=1000): for _ in range(num_round_trips): id_orig = id(recycled) recycled = m.unique_ptr_roundtrip(recycled) - assert re.match("passenger(_MvCtor)*_MvCtor", m.get_mtxt(recycled)) + check_regex("passenger(_MvCtor){1,2}", m.get_mtxt(recycled)) id_rtrn = id(recycled) # Ensure the returned object is a different Python instance. assert id_rtrn != id_orig @@ -132,7 +138,7 @@ def test_unique_ptr_consumer_roundtrip(pass_f, rtrn_f, moved_out, moved_in): recycled = m.atyp("passenger") mtxt_orig = m.get_mtxt(recycled) ptr_orig = m.get_ptr(recycled) - assert re.match("passenger_(MvCtor){1,2}", mtxt_orig) + check_regex("passenger(_MvCtor){1,2}", mtxt_orig) pass_f(c, recycled) # pass object to C++ consumer c if moved_out: # if moved (always), ensure it is flagged as disowned