From 5a564ca128ad76223fd7c8c3075ef6cd0ff2ad19 Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Wed, 5 Apr 2023 17:41:17 -0700 Subject: [PATCH 1/5] Avoid dangling pointers. --- .../detail/smart_holder_type_casters.h | 6 ++- tests/test_return_value_policy_override.cpp | 45 +++++++++++++++++++ tests/test_return_value_policy_override.py | 13 ++++++ 3 files changed, 63 insertions(+), 1 deletion(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 5e0f791c3c..2b96512b1f 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -715,7 +715,11 @@ 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; + if (parent) { + policy = return_value_policy::reference_internal; + } else { + policy = return_value_policy::reference; + } } return cast(const_cast(src), policy, parent); // Mutbl2Const } diff --git a/tests/test_return_value_policy_override.cpp b/tests/test_return_value_policy_override.cpp index 5d6a51b7c4..2be2989f9c 100644 --- a/tests/test_return_value_policy_override.cpp +++ b/tests/test_return_value_policy_override.cpp @@ -2,10 +2,36 @@ #include "pybind11_tests.h" +#include + namespace test_return_value_policy_override { struct some_type {}; +struct data_field { + int value = -99; +}; + +struct data_fields_holder { + std::vector vec; + + explicit data_fields_holder(std::size_t vec_size) { + for (std::size_t i = 0; i < vec_size; i++) { + vec.push_back(data_field{13 + static_cast(i) * 11}); + } + } + + data_field *vec_at(std::size_t index) { + if (index >= vec.size()) { + return nullptr; + } else { + return &vec[index]; + } + } + + const data_field *vec_at_const_ptr(std::size_t index) { return vec_at(index); } +}; + // cp = copyable, mv = movable, 1 = yes, 0 = no struct type_cp1_mv1 { std::string mtxt; @@ -156,6 +182,8 @@ std::unique_ptr return_unique_pointer_nocopy_nomove() { } // namespace test_return_value_policy_override +using test_return_value_policy_override::data_field; +using test_return_value_policy_override::data_fields_holder; 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; @@ -205,6 +233,8 @@ struct type_caster : type_caster_base { } // namespace detail } // namespace pybind11 +PYBIND11_SMART_HOLDER_TYPE_CASTERS(data_field) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(data_fields_holder) PYBIND11_SMART_HOLDER_TYPE_CASTERS(type_cp1_mv1) PYBIND11_SMART_HOLDER_TYPE_CASTERS(type_cp0_mv1) PYBIND11_SMART_HOLDER_TYPE_CASTERS(type_cp1_mv0) @@ -239,6 +269,21 @@ TEST_SUBMODULE(return_value_policy_override, m) { }, py::return_value_policy::_clif_automatic); + py::classh(m, "data_field").def_readwrite("value", &data_field::value); + py::classh(m, "data_fields_holder") + .def(py::init()) + .def("vec_at", + [](py::object self_py, std::size_t index) { + auto self = py::cast(self_py); + return py::cast( + self->vec_at(index), py::return_value_policy::_clif_automatic, self_py); + }) + .def("vec_at_const_ptr", [](py::object self_py, std::size_t index) { + auto self = py::cast(self_py); + return py::cast( + self->vec_at_const_ptr(index), py::return_value_policy::_clif_automatic, self_py); + }); + py::classh(m, "type_cp1_mv1") .def(py::init()) .def_readonly("mtxt", &type_cp1_mv1::mtxt); diff --git a/tests/test_return_value_policy_override.py b/tests/test_return_value_policy_override.py index 27c7694213..1412588810 100644 --- a/tests/test_return_value_policy_override.py +++ b/tests/test_return_value_policy_override.py @@ -17,6 +17,19 @@ def test_return_pointer(): assert m.return_pointer_with_policy_clif_automatic() == "_clif_automatic" +def test_persistent_holder(): + h = m.data_fields_holder(2) + assert h.vec_at(0).value == 13 + assert h.vec_at(1).value == 24 + + +def test_temporary_holder(): + data_field = m.data_fields_holder(2).vec_at(1) + assert data_field.value == 24 + data_field = m.data_fields_holder(2).vec_at_const_ptr(1) + assert data_field.value == 24 + + @pytest.mark.parametrize( ("func", "expected"), [ From 3deca7bd76405a15221991d53e0472b04bf022a2 Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Wed, 5 Apr 2023 17:45:41 -0700 Subject: [PATCH 2/5] Add test for const ptr --- tests/test_return_value_policy_override.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_return_value_policy_override.py b/tests/test_return_value_policy_override.py index 1412588810..213f9c20e3 100644 --- a/tests/test_return_value_policy_override.py +++ b/tests/test_return_value_policy_override.py @@ -21,6 +21,8 @@ def test_persistent_holder(): h = m.data_fields_holder(2) assert h.vec_at(0).value == 13 assert h.vec_at(1).value == 24 + assert h.vec_at_const_ptr(0).value == 13 + assert h.vec_at_const_ptr(1).value == 24 def test_temporary_holder(): From 4ea110a45d1e1c73ff9e9e848f94b2b6c19b178e Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Wed, 5 Apr 2023 18:03:18 -0700 Subject: [PATCH 3/5] Fix test failures. --- tests/test_return_value_policy_override.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_return_value_policy_override.cpp b/tests/test_return_value_policy_override.cpp index 2be2989f9c..fbc0d609ad 100644 --- a/tests/test_return_value_policy_override.cpp +++ b/tests/test_return_value_policy_override.cpp @@ -10,6 +10,8 @@ struct some_type {}; struct data_field { int value = -99; + + explicit data_field(int v) : value(v) {} }; struct data_fields_holder { From c91298b59e7f16d0656ca65902cd2d47716f58da Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Thu, 6 Apr 2023 11:55:59 -0700 Subject: [PATCH 4/5] Fix ClangTidy --- tests/test_return_value_policy_override.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/tests/test_return_value_policy_override.cpp b/tests/test_return_value_policy_override.cpp index fbc0d609ad..5597a3bdc1 100644 --- a/tests/test_return_value_policy_override.cpp +++ b/tests/test_return_value_policy_override.cpp @@ -19,16 +19,15 @@ struct data_fields_holder { explicit data_fields_holder(std::size_t vec_size) { for (std::size_t i = 0; i < vec_size; i++) { - vec.push_back(data_field{13 + static_cast(i) * 11}); + vec.emplace_back(data_field{13 + static_cast(i) * 11}); } } data_field *vec_at(std::size_t index) { if (index >= vec.size()) { return nullptr; - } else { - return &vec[index]; } + return &vec[index]; } const data_field *vec_at_const_ptr(std::size_t index) { return vec_at(index); } @@ -275,13 +274,13 @@ TEST_SUBMODULE(return_value_policy_override, m) { py::classh(m, "data_fields_holder") .def(py::init()) .def("vec_at", - [](py::object self_py, std::size_t index) { - auto self = py::cast(self_py); + [](const py::object &self_py, std::size_t index) { + auto *self = py::cast(self_py); return py::cast( self->vec_at(index), py::return_value_policy::_clif_automatic, self_py); }) - .def("vec_at_const_ptr", [](py::object self_py, std::size_t index) { - auto self = py::cast(self_py); + .def("vec_at_const_ptr", [](const py::object &self_py, std::size_t index) { + auto *self = py::cast(self_py); return py::cast( self->vec_at_const_ptr(index), py::return_value_policy::_clif_automatic, self_py); }); From 552f900c850dbd532320ef9b1cdb004be32dcb8d Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Thu, 6 Apr 2023 12:14:55 -0700 Subject: [PATCH 5/5] fix emplace_back --- tests/test_return_value_policy_override.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_return_value_policy_override.cpp b/tests/test_return_value_policy_override.cpp index 5597a3bdc1..a61bec1ba6 100644 --- a/tests/test_return_value_policy_override.cpp +++ b/tests/test_return_value_policy_override.cpp @@ -19,7 +19,7 @@ struct data_fields_holder { explicit data_fields_holder(std::size_t vec_size) { for (std::size_t i = 0; i < vec_size; i++) { - vec.emplace_back(data_field{13 + static_cast(i) * 11}); + vec.emplace_back(13 + static_cast(i) * 11); } }