From c936890d24b17d870fe3f71ddc9d6f0b7110b05b Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 6 May 2021 12:30:42 -0700 Subject: [PATCH 01/39] WIP stash-kind-of-commit --- include/pybind11/detail/smart_holder_poc.h | 7 ++ tests/test_class_sh_nested_fields.cpp | 89 ++++++++++++++++++++++ tests/test_class_sh_nested_fields.py | 40 ++++++++++ 3 files changed, 136 insertions(+) create mode 100644 tests/test_class_sh_nested_fields.cpp create mode 100644 tests/test_class_sh_nested_fields.py diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h index 4a2dc034c2..efe398f81d 100644 --- a/include/pybind11/detail/smart_holder_poc.h +++ b/include/pybind11/detail/smart_holder_poc.h @@ -54,6 +54,9 @@ High-level aspects: #include #include +#include +inline void to_cout(std::string msg) { std::cout << msg << std::endl; } + // pybindit = Python Bindings Innovation Track. // Currently not in pybind11 namespace to signal that this POC does not depend // on any existing pybind11 functionality. @@ -216,6 +219,7 @@ struct smart_holder { } static smart_holder from_raw_ptr_unowned(void *raw_ptr) { + to_cout("LOOOK " + std::to_string(__LINE__)); smart_holder hld; hld.vptr.reset(raw_ptr, [](void *) {}); hld.vptr_is_using_noop_deleter = true; @@ -246,6 +250,7 @@ struct smart_holder { template static smart_holder from_raw_ptr_take_ownership(T *raw_ptr, bool void_cast_raw_ptr = false) { + to_cout("LOOOK " + std::to_string(__LINE__)); ensure_pointee_is_destructible("from_raw_ptr_take_ownership"); smart_holder hld; auto gd = make_guarded_builtin_delete(true); @@ -297,6 +302,7 @@ struct smart_holder { template static smart_holder from_unique_ptr(std::unique_ptr &&unq_ptr, bool void_cast_raw_ptr = false) { + to_cout("LOOOK " + std::to_string(__LINE__)); smart_holder hld; hld.rtti_uqp_del = &typeid(D); hld.vptr_is_using_builtin_delete = is_std_default_delete(*hld.rtti_uqp_del); @@ -326,6 +332,7 @@ struct smart_holder { template static smart_holder from_shared_ptr(std::shared_ptr shd_ptr) { + to_cout("LOOOK " + std::to_string(__LINE__)); smart_holder hld; hld.vptr = std::static_pointer_cast(shd_ptr); hld.vptr_is_external_shared_ptr = true; diff --git a/tests/test_class_sh_nested_fields.cpp b/tests/test_class_sh_nested_fields.cpp new file mode 100644 index 0000000000..6876435f24 --- /dev/null +++ b/tests/test_class_sh_nested_fields.cpp @@ -0,0 +1,89 @@ +#include "pybind11_tests.h" + +#include "pybind11/smart_holder.h" + +#include +#include + +namespace { + +struct DD { + int i; +}; + +struct CC { + int i = 13; +}; + +struct BB { + CC c; + DD d; + + DD GetD() { return d; } + + void SetD(const DD &dd) { d = dd; } +}; + +struct AA { + BB b; + CC c; + DD *dp; + std::unique_ptr
du; + std::shared_ptr
ds; + std::vector v; +}; + +inline void ConsumeCC(std::unique_ptr) {} +inline void ConsumeAA(std::unique_ptr) {} + +} // namespace + +PYBIND11_SMART_HOLDER_TYPE_CASTERS(DD) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(CC) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(BB) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(AA) + +TEST_SUBMODULE(class_sh_nested_fields, m) { + m.def("to_cout", to_cout); + + py::classh<::DD> DD_class(m, "DD"); + DD_class.def_readwrite("i", &::DD::i); + DD_class.def(py::init<>()); + + py::classh<::CC> CC_class(m, "CC"); + CC_class.def_readwrite("i", &::CC::i); + CC_class.def(py::init<>()); + + py::classh<::BB> BB_class(m, "BB"); + BB_class.def_readwrite("c", &::BB::c); + BB_class.def_property("d", &::BB::GetD, &::BB::SetD); + BB_class.def(py::init<>()); + + py::classh<::AA> AA_class(m, "AA"); + AA_class.def_readwrite("b", &::AA::b); + AA_class.def_readwrite("v", &::AA::v); + AA_class.def("GetC", [](::AA &self) { return self.c; }); + AA_class.def("SetC", [](::AA &self, ::CC v) { self.c = v; }); + AA_class.def_property( + "dp", + [](::AA &self) { return *self.dp; }, + [](::AA &self, ::DD *v) { self.dp = v; }, + py::return_value_policy::reference_internal); + AA_class.def_property( + "du", + [](::AA &self) { return *self.du; }, + [](::AA &self, ::std::unique_ptr<::DD> v) { self.du = std::move(v); }, + py::return_value_policy::reference_internal); + AA_class.def_readwrite("ds", &::AA::ds); + AA_class.def(py::init<>()); + + m.def("ConsumeCC", + (void (*)(::std::unique_ptr<::CC>)) & ::ConsumeCC, + py::arg("cc"), + py::return_value_policy::automatic); + + m.def("ConsumeAA", + (void (*)(::std::unique_ptr<::AA>)) & ::ConsumeAA, + py::arg("aa"), + py::return_value_policy::automatic); +} diff --git a/tests/test_class_sh_nested_fields.py b/tests/test_class_sh_nested_fields.py new file mode 100644 index 0000000000..99c153881f --- /dev/null +++ b/tests/test_class_sh_nested_fields.py @@ -0,0 +1,40 @@ +# -*- coding: utf-8 -*- +import pytest + +from pybind11_tests import class_sh_nested_fields as m + + +def test_bc(msg): + m.to_cout("") + m.to_cout("HELLO") + b = m.BB() + m.to_cout("have b") + c = b.c + m.to_cout("have c") + with pytest.raises(ValueError) as excinfo: + m.ConsumeCC(c) + assert ( + msg(excinfo.value) == "Cannot disown non-owning holder (loaded_as_unique_ptr)." + ) + + +def test_abc(): + m.to_cout("") + m.to_cout("LOOOK TEST_ABC") + a = m.AA() + m.to_cout("LOOOK have a") + b = a.b + m.to_cout("LOOOK have b") + c = b.c + m.to_cout("LOOOK have c") + assert c.i == 13 + m.to_cout("LOOOK c.i reset") + m.ConsumeAA(a) # Without this it works. + m.to_cout("LOOOK ConsumeAA done") + assert ( + c.i == 13 + ) # AddressSanitizer: heap-use-after-free pybind11.h:235:52 in operator() + # 233 /* Perform the function call */ + # 234 handle result = cast_out::cast( + # 235 std::move(args_converter).template call(cap->f), policy, call.parent); + m.to_cout("LOOOK c.i equality done") From f1625dcdeb86fa85c92fcf2a3e21216f99ef65ab Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 29 Dec 2021 12:32:16 -0800 Subject: [PATCH 02/39] Minimal reproducer. --- include/pybind11/detail/smart_holder_poc.h | 7 -- tests/test_class_sh_nested_fields.cpp | 89 ---------------------- tests/test_class_sh_nested_fields.py | 40 ---------- tests/test_class_sh_property.cpp | 35 +++++++++ tests/test_class_sh_property.py | 10 +++ 5 files changed, 45 insertions(+), 136 deletions(-) delete mode 100644 tests/test_class_sh_nested_fields.cpp delete mode 100644 tests/test_class_sh_nested_fields.py create mode 100644 tests/test_class_sh_property.cpp create mode 100644 tests/test_class_sh_property.py diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h index efe398f81d..4a2dc034c2 100644 --- a/include/pybind11/detail/smart_holder_poc.h +++ b/include/pybind11/detail/smart_holder_poc.h @@ -54,9 +54,6 @@ High-level aspects: #include #include -#include -inline void to_cout(std::string msg) { std::cout << msg << std::endl; } - // pybindit = Python Bindings Innovation Track. // Currently not in pybind11 namespace to signal that this POC does not depend // on any existing pybind11 functionality. @@ -219,7 +216,6 @@ struct smart_holder { } static smart_holder from_raw_ptr_unowned(void *raw_ptr) { - to_cout("LOOOK " + std::to_string(__LINE__)); smart_holder hld; hld.vptr.reset(raw_ptr, [](void *) {}); hld.vptr_is_using_noop_deleter = true; @@ -250,7 +246,6 @@ struct smart_holder { template static smart_holder from_raw_ptr_take_ownership(T *raw_ptr, bool void_cast_raw_ptr = false) { - to_cout("LOOOK " + std::to_string(__LINE__)); ensure_pointee_is_destructible("from_raw_ptr_take_ownership"); smart_holder hld; auto gd = make_guarded_builtin_delete(true); @@ -302,7 +297,6 @@ struct smart_holder { template static smart_holder from_unique_ptr(std::unique_ptr &&unq_ptr, bool void_cast_raw_ptr = false) { - to_cout("LOOOK " + std::to_string(__LINE__)); smart_holder hld; hld.rtti_uqp_del = &typeid(D); hld.vptr_is_using_builtin_delete = is_std_default_delete(*hld.rtti_uqp_del); @@ -332,7 +326,6 @@ struct smart_holder { template static smart_holder from_shared_ptr(std::shared_ptr shd_ptr) { - to_cout("LOOOK " + std::to_string(__LINE__)); smart_holder hld; hld.vptr = std::static_pointer_cast(shd_ptr); hld.vptr_is_external_shared_ptr = true; diff --git a/tests/test_class_sh_nested_fields.cpp b/tests/test_class_sh_nested_fields.cpp deleted file mode 100644 index 6876435f24..0000000000 --- a/tests/test_class_sh_nested_fields.cpp +++ /dev/null @@ -1,89 +0,0 @@ -#include "pybind11_tests.h" - -#include "pybind11/smart_holder.h" - -#include -#include - -namespace { - -struct DD { - int i; -}; - -struct CC { - int i = 13; -}; - -struct BB { - CC c; - DD d; - - DD GetD() { return d; } - - void SetD(const DD &dd) { d = dd; } -}; - -struct AA { - BB b; - CC c; - DD *dp; - std::unique_ptr
du; - std::shared_ptr
ds; - std::vector v; -}; - -inline void ConsumeCC(std::unique_ptr) {} -inline void ConsumeAA(std::unique_ptr) {} - -} // namespace - -PYBIND11_SMART_HOLDER_TYPE_CASTERS(DD) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(CC) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(BB) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(AA) - -TEST_SUBMODULE(class_sh_nested_fields, m) { - m.def("to_cout", to_cout); - - py::classh<::DD> DD_class(m, "DD"); - DD_class.def_readwrite("i", &::DD::i); - DD_class.def(py::init<>()); - - py::classh<::CC> CC_class(m, "CC"); - CC_class.def_readwrite("i", &::CC::i); - CC_class.def(py::init<>()); - - py::classh<::BB> BB_class(m, "BB"); - BB_class.def_readwrite("c", &::BB::c); - BB_class.def_property("d", &::BB::GetD, &::BB::SetD); - BB_class.def(py::init<>()); - - py::classh<::AA> AA_class(m, "AA"); - AA_class.def_readwrite("b", &::AA::b); - AA_class.def_readwrite("v", &::AA::v); - AA_class.def("GetC", [](::AA &self) { return self.c; }); - AA_class.def("SetC", [](::AA &self, ::CC v) { self.c = v; }); - AA_class.def_property( - "dp", - [](::AA &self) { return *self.dp; }, - [](::AA &self, ::DD *v) { self.dp = v; }, - py::return_value_policy::reference_internal); - AA_class.def_property( - "du", - [](::AA &self) { return *self.du; }, - [](::AA &self, ::std::unique_ptr<::DD> v) { self.du = std::move(v); }, - py::return_value_policy::reference_internal); - AA_class.def_readwrite("ds", &::AA::ds); - AA_class.def(py::init<>()); - - m.def("ConsumeCC", - (void (*)(::std::unique_ptr<::CC>)) & ::ConsumeCC, - py::arg("cc"), - py::return_value_policy::automatic); - - m.def("ConsumeAA", - (void (*)(::std::unique_ptr<::AA>)) & ::ConsumeAA, - py::arg("aa"), - py::return_value_policy::automatic); -} diff --git a/tests/test_class_sh_nested_fields.py b/tests/test_class_sh_nested_fields.py deleted file mode 100644 index 99c153881f..0000000000 --- a/tests/test_class_sh_nested_fields.py +++ /dev/null @@ -1,40 +0,0 @@ -# -*- coding: utf-8 -*- -import pytest - -from pybind11_tests import class_sh_nested_fields as m - - -def test_bc(msg): - m.to_cout("") - m.to_cout("HELLO") - b = m.BB() - m.to_cout("have b") - c = b.c - m.to_cout("have c") - with pytest.raises(ValueError) as excinfo: - m.ConsumeCC(c) - assert ( - msg(excinfo.value) == "Cannot disown non-owning holder (loaded_as_unique_ptr)." - ) - - -def test_abc(): - m.to_cout("") - m.to_cout("LOOOK TEST_ABC") - a = m.AA() - m.to_cout("LOOOK have a") - b = a.b - m.to_cout("LOOOK have b") - c = b.c - m.to_cout("LOOOK have c") - assert c.i == 13 - m.to_cout("LOOOK c.i reset") - m.ConsumeAA(a) # Without this it works. - m.to_cout("LOOOK ConsumeAA done") - assert ( - c.i == 13 - ) # AddressSanitizer: heap-use-after-free pybind11.h:235:52 in operator() - # 233 /* Perform the function call */ - # 234 handle result = cast_out::cast( - # 235 std::move(args_converter).template call(cap->f), policy, call.parent); - m.to_cout("LOOOK c.i equality done") diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp new file mode 100644 index 0000000000..dec67749df --- /dev/null +++ b/tests/test_class_sh_property.cpp @@ -0,0 +1,35 @@ +#include "pybind11_tests.h" + +#include "pybind11/smart_holder.h" + +#include + +namespace { + +struct Inner { + int value = -99; +}; + +struct Outer { + Inner field; +}; + +inline void DisownOuter(std::unique_ptr) {} + +} // namespace + +PYBIND11_SMART_HOLDER_TYPE_CASTERS(Inner) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(Outer) + +TEST_SUBMODULE(class_sh_property, m) { + py::classh(m, "Inner") // + .def_readwrite("value", &Inner::value) // + ; + + py::classh(m, "Outer") // + .def(py::init<>()) // + .def_readwrite("field", &Outer::field) // + ; + + m.def("DisownOuter", DisownOuter); +} diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py new file mode 100644 index 0000000000..15f51d37c4 --- /dev/null +++ b/tests/test_class_sh_property.py @@ -0,0 +1,10 @@ +# -*- coding: utf-8 -*- +from pybind11_tests import class_sh_property as m + + +def test_inner_access_after_disowning_outer(): + outer = m.Outer() + inner = outer.field + assert inner.value == -99 + m.DisownOuter(outer) + # assert inner.value == -99 # AddressSanitizer: heap-use-after-free From 94c13ca5716e78cd22733eafb2afb75469cc829f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 29 Dec 2021 12:42:08 -0800 Subject: [PATCH 03/39] Using named namespace to fix double registration error in Google environment. --- tests/test_class_sh_property.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index dec67749df..f18dcaa34d 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -4,7 +4,7 @@ #include -namespace { +namespace test_class_sh_property { struct Inner { int value = -99; @@ -16,12 +16,14 @@ struct Outer { inline void DisownOuter(std::unique_ptr) {} -} // namespace +} // namespace test_class_sh_property -PYBIND11_SMART_HOLDER_TYPE_CASTERS(Inner) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(Outer) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_property::Inner) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_property::Outer) TEST_SUBMODULE(class_sh_property, m) { + using namespace test_class_sh_property; + py::classh(m, "Inner") // .def_readwrite("value", &Inner::value) // ; From 6082ec4bfa7d273cd3d6735091691b76230651c9 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 29 Dec 2021 12:48:10 -0800 Subject: [PATCH 04/39] Adding link to PyCLIF test this was reduced from. --- tests/test_class_sh_property.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index 15f51d37c4..eae6cae4a4 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -2,6 +2,8 @@ from pybind11_tests import class_sh_property as m +# Reduced from: +# https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/testing/python/nested_fields_test.py#L56 def test_inner_access_after_disowning_outer(): outer = m.Outer() inner = outer.field From 9b9e1bb13d52ae4a26311bd03eb9dc5219a983e3 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 30 Dec 2021 07:51:53 -0800 Subject: [PATCH 05/39] Using aliasing shared_ptr in field getter. Emulating PyCLIF approach: https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233 --- tests/CMakeLists.txt | 111 +++++++++++++++---------------- tests/test_class_sh_property.cpp | 15 ++++- tests/test_class_sh_property.py | 28 ++++++-- 3 files changed, 91 insertions(+), 63 deletions(-) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 1c59c193bc..90485300e2 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -115,62 +115,61 @@ endif() # Any test that has no extension is both .py and .cpp, so 'foo' will add 'foo.cpp' and 'foo.py'. # Any test that has an extension is exclusively that and handled as such. set(PYBIND11_TEST_FILES - test_async - test_buffers - test_builtin_casters - test_call_policies - test_callbacks - test_chrono - test_class - test_class_sh_basic - test_class_sh_disowning - test_class_sh_disowning_mi - test_class_sh_factory_constructors - test_class_sh_inheritance - test_class_sh_module_local.py - test_class_sh_shared_ptr_copy_move - test_class_sh_trampoline_basic - test_class_sh_trampoline_self_life_support - test_class_sh_trampoline_shared_from_this - test_class_sh_trampoline_shared_ptr_cpp_arg - test_class_sh_trampoline_unique_ptr - test_class_sh_unique_ptr_member - test_class_sh_virtual_py_cpp_mix - test_class_sh_void_ptr_capsule - test_classh_mock - test_const_name - test_constants_and_functions - test_copy_move - test_custom_type_casters - test_custom_type_setup - test_docstring_options - test_eigen - test_enum - test_eval - test_exceptions - test_factory_constructors - test_gil_scoped - test_iostream - test_kwargs_and_defaults - test_local_bindings - test_methods_and_attributes - test_modules - test_multiple_inheritance - test_numpy_array - test_numpy_dtypes - test_numpy_vectorize - test_opaque_types - test_operator_overloading - test_pickling - test_pytypes - test_sequences_and_iterators - test_smart_ptr - test_stl - test_stl_binders - test_tagbased_polymorphic - test_thread - test_union - test_virtual_functions) + test_async.cpp + test_buffers.cpp + test_builtin_casters.cpp + test_call_policies.cpp + test_callbacks.cpp + test_chrono.cpp + test_class.cpp + test_class_sh_basic.cpp + test_class_sh_disowning.cpp + test_class_sh_disowning_mi.cpp + test_class_sh_factory_constructors.cpp + test_class_sh_inheritance.cpp + test_class_sh_property.cpp + test_class_sh_shared_ptr_copy_move.cpp + test_class_sh_trampoline_basic.cpp + test_class_sh_trampoline_self_life_support.cpp + test_class_sh_trampoline_shared_from_this.cpp + test_class_sh_trampoline_shared_ptr_cpp_arg.cpp + test_class_sh_trampoline_unique_ptr.cpp + test_class_sh_unique_ptr_member.cpp + test_class_sh_virtual_py_cpp_mix.cpp + test_classh_mock.cpp + test_const_name.cpp + test_constants_and_functions.cpp + test_copy_move.cpp + test_custom_type_casters.cpp + test_custom_type_setup.cpp + test_docstring_options.cpp + test_eigen.cpp + test_enum.cpp + test_eval.cpp + test_exceptions.cpp + test_factory_constructors.cpp + test_gil_scoped.cpp + test_iostream.cpp + test_kwargs_and_defaults.cpp + test_local_bindings.cpp + test_methods_and_attributes.cpp + test_modules.cpp + test_multiple_inheritance.cpp + test_numpy_array.cpp + test_numpy_dtypes.cpp + test_numpy_vectorize.cpp + test_opaque_types.cpp + test_operator_overloading.cpp + test_pickling.cpp + test_pytypes.cpp + test_sequences_and_iterators.cpp + test_smart_ptr.cpp + test_stl.cpp + test_stl_binders.cpp + test_tagbased_polymorphic.cpp + test_thread.cpp + test_union.cpp + test_virtual_functions.cpp) # Invoking cmake with something like: # cmake -DPYBIND11_TEST_OVERRIDE="test_callbacks.cpp;test_pickling.cpp" .. diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index f18dcaa34d..23aa902983 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -25,12 +25,21 @@ TEST_SUBMODULE(class_sh_property, m) { using namespace test_class_sh_property; py::classh(m, "Inner") // + .def(py::init<>()) // .def_readwrite("value", &Inner::value) // ; - py::classh(m, "Outer") // - .def(py::init<>()) // - .def_readwrite("field", &Outer::field) // + py::classh(m, "Outer") // + .def(py::init<>()) // + .def_property( + "field", // + [](const std::shared_ptr &self) { + // Emulating PyCLIF approach: + // https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233 + return std::shared_ptr(self, &self->field); + }, // + [](Outer &self, const Inner &field) { self.field = field; } // + ) // ; m.def("DisownOuter", DisownOuter); diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index eae6cae4a4..8a5b879880 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -1,12 +1,32 @@ # -*- coding: utf-8 -*- +import pytest + from pybind11_tests import class_sh_property as m -# Reduced from: -# https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/testing/python/nested_fields_test.py#L56 -def test_inner_access_after_disowning_outer(): +def test_field_getter(msg): + # Reduced from PyCLIF test: + # https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/testing/python/nested_fields_test.py#L56 outer = m.Outer() inner = outer.field assert inner.value == -99 + with pytest.raises(ValueError) as excinfo: + m.DisownOuter(outer) + assert msg(excinfo.value) == "Cannot disown use_count != 1 (loaded_as_unique_ptr)." + del inner m.DisownOuter(outer) - # assert inner.value == -99 # AddressSanitizer: heap-use-after-free + with pytest.raises(ValueError) as excinfo: + outer.field + assert ( + msg(excinfo.value) + == "Missing value for wrapped C++ type: Python instance was disowned." + ) + + +def test_field_setter(msg): + outer = m.Outer() + assert outer.field.value == -99 + field = m.Inner() + field.value = 35 + outer.field = field + assert outer.field.value == 35 From c14612003975fe1f3bf9e2ef4060e34c4d0323ff Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 30 Dec 2021 08:14:56 -0800 Subject: [PATCH 06/39] PyPy xfail --- tests/test_class_sh_property.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index 8a5b879880..5d293f8ff9 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -1,9 +1,11 @@ # -*- coding: utf-8 -*- import pytest +import env # noqa: F401 from pybind11_tests import class_sh_property as m +@pytest.mark.xfail("env.PYPY", reason="gc after `del inner` is apparently deferred") def test_field_getter(msg): # Reduced from PyCLIF test: # https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/testing/python/nested_fields_test.py#L56 From 4a9228f591c5fa8d6216e795dba5fa367f360d1d Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 2 Jan 2022 16:32:32 -0800 Subject: [PATCH 07/39] Renaming in preparation for adding `std::shared_ptr`. --- include/pybind11/pybind11.h | 39 ++++++++++++++++++++++++++++---- tests/test_class_sh_property.cpp | 26 +++++++++++++-------- tests/test_class_sh_property.py | 20 ++++++++-------- 3 files changed, 60 insertions(+), 25 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 8fe26a8454..bf2b04d974 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1377,6 +1377,32 @@ using default_holder_type = smart_holder; } #endif + +template +struct getter_cpp_function { + template + static cpp_function make(PM pm, M m) { return cpp_function([pm](const T &c) -> const D &{ return c.*pm; }, m); } +}; + +#ifdef JUNK +template +struct getter_cpp_function::value && + detail::type_uses_smart_holder_type_caster::value>> { + template + static cpp_function make(PM pm, M m) { + return cpp_function([pm](const std::shared_ptr &c_sp) -> std::shared_ptr{ + const T &c = *c_sp.get(); + if constexpr (std::is_same, decltype(c.*pm)>::value) { + return c.*pm; + } + D &d = const_cast(c.*pm); + return std::shared_ptr(c_sp, &d); + }, + m); + } +}; +#endif + // clang-format off template @@ -1577,9 +1603,12 @@ class class_ : public detail::generic_type { template class_ &def_readwrite(const char *name, D C::*pm, const Extra&... extra) { static_assert(std::is_same::value || std::is_base_of::value, "def_readwrite() requires a class member (or base class member)"); - cpp_function fget([pm](const type &c) -> const D &{ return c.*pm; }, is_method(*this)), - fset([pm](type &c, const D &value) { c.*pm = value; }, is_method(*this)); - def_property(name, fget, fset, return_value_policy::reference_internal, extra...); + def_property( + name, + getter_cpp_function::make(pm, is_method(*this)), + cpp_function([pm](type &c, const D &value) { c.*pm = value; }, is_method(*this)), + return_value_policy::reference_internal, + extra...); return *this; } @@ -1593,8 +1622,8 @@ class class_ : public detail::generic_type { template class_ &def_readwrite_static(const char *name, D *pm, const Extra& ...extra) { - cpp_function fget([pm](const object &) -> const D & { return *pm; }, scope(*this)), - fset([pm](const object &, const D &value) { *pm = value; }, scope(*this)); + cpp_function fget([pm](const object &) -> const D & { return *pm; }, scope(*this)); + cpp_function fset([pm](const object &, const D &value) { *pm = value; }, scope(*this)); def_property_static(name, fget, fset, return_value_policy::reference, extra...); return *this; } diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index 23aa902983..e3547c8b43 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -6,40 +6,46 @@ namespace test_class_sh_property { -struct Inner { - int value = -99; +struct Field { + int num = -99; }; struct Outer { - Inner field; + Field m_val; + std::shared_ptr m_sh_ptr; }; inline void DisownOuter(std::unique_ptr) {} } // namespace test_class_sh_property -PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_property::Inner) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_property::Field) PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_property::Outer) TEST_SUBMODULE(class_sh_property, m) { using namespace test_class_sh_property; - py::classh(m, "Inner") // - .def(py::init<>()) // - .def_readwrite("value", &Inner::value) // + py::classh(m, "Field") // + .def(py::init<>()) // + .def_readwrite("num", &Field::num) // ; py::classh(m, "Outer") // .def(py::init<>()) // +#ifdef JUNK + .def_readwrite("m_val", &Outer::m_val) // + .def_readwrite("m_sh_ptr", &Outer::m_sh_ptr) // +#else .def_property( - "field", // + "m_val", // [](const std::shared_ptr &self) { // Emulating PyCLIF approach: // https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233 - return std::shared_ptr(self, &self->field); + return std::shared_ptr(self, &self->m_val); }, // - [](Outer &self, const Inner &field) { self.field = field; } // + [](Outer &self, const Field &m_val) { self.m_val = m_val; } // ) // +#endif ; m.def("DisownOuter", DisownOuter); diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index 5d293f8ff9..76765012b9 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -5,20 +5,20 @@ from pybind11_tests import class_sh_property as m -@pytest.mark.xfail("env.PYPY", reason="gc after `del inner` is apparently deferred") +@pytest.mark.xfail("env.PYPY", reason="gc after `del field` is apparently deferred") def test_field_getter(msg): # Reduced from PyCLIF test: # https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/testing/python/nested_fields_test.py#L56 outer = m.Outer() - inner = outer.field - assert inner.value == -99 + field = outer.m_val + assert field.num == -99 with pytest.raises(ValueError) as excinfo: m.DisownOuter(outer) assert msg(excinfo.value) == "Cannot disown use_count != 1 (loaded_as_unique_ptr)." - del inner + del field m.DisownOuter(outer) with pytest.raises(ValueError) as excinfo: - outer.field + outer.m_val assert ( msg(excinfo.value) == "Missing value for wrapped C++ type: Python instance was disowned." @@ -27,8 +27,8 @@ def test_field_getter(msg): def test_field_setter(msg): outer = m.Outer() - assert outer.field.value == -99 - field = m.Inner() - field.value = 35 - outer.field = field - assert outer.field.value == 35 + assert outer.m_val.num == -99 + field = m.Field() + field.num = 35 + outer.m_val = field + assert outer.m_val.num == 35 From 526c8d6f7cf2c2ecfda9ba73d69430fa189be626 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 3 Jan 2022 16:33:13 -0800 Subject: [PATCH 08/39] Adding is_shared_ptr condition (all tests work SH_AVL, SH_DEF). --- include/pybind11/pybind11.h | 15 +++++++++------ tests/test_class_sh_property.cpp | 12 ------------ 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index bf2b04d974..5c6c789ae7 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1384,24 +1384,27 @@ struct getter_cpp_function { static cpp_function make(PM pm, M m) { return cpp_function([pm](const T &c) -> const D &{ return c.*pm; }, m); } }; -#ifdef JUNK +template +struct is_std_shared_ptr : std::false_type {}; +template +struct is_std_shared_ptr> : std::true_type {}; + template struct getter_cpp_function::value && - detail::type_uses_smart_holder_type_caster::value>> { + detail::type_uses_smart_holder_type_caster::value && + !is_std_shared_ptr::value>> { template static cpp_function make(PM pm, M m) { return cpp_function([pm](const std::shared_ptr &c_sp) -> std::shared_ptr{ const T &c = *c_sp.get(); - if constexpr (std::is_same, decltype(c.*pm)>::value) { - return c.*pm; - } D &d = const_cast(c.*pm); + // Emulating PyCLIF approach: + // https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233 return std::shared_ptr(c_sp, &d); }, m); } }; -#endif // clang-format off diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index e3547c8b43..0c7d5b5d45 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -32,20 +32,8 @@ TEST_SUBMODULE(class_sh_property, m) { py::classh(m, "Outer") // .def(py::init<>()) // -#ifdef JUNK .def_readwrite("m_val", &Outer::m_val) // .def_readwrite("m_sh_ptr", &Outer::m_sh_ptr) // -#else - .def_property( - "m_val", // - [](const std::shared_ptr &self) { - // Emulating PyCLIF approach: - // https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233 - return std::shared_ptr(self, &self->m_val); - }, // - [](Outer &self, const Field &m_val) { self.m_val = m_val; } // - ) // -#endif ; m.def("DisownOuter", DisownOuter); From 5cfd5291e84ddd8b870da9bfe4440c3b51d82fc9 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 3 Jan 2022 16:37:19 -0800 Subject: [PATCH 09/39] clang-format --- include/pybind11/pybind11.h | 30 ++++++++++++++++++------------ tests/test_class_sh_property.cpp | 6 +++--- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 5c6c789ae7..94a9e363af 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1381,7 +1381,9 @@ using default_holder_type = smart_holder; template struct getter_cpp_function { template - static cpp_function make(PM pm, M m) { return cpp_function([pm](const T &c) -> const D &{ return c.*pm; }, m); } + static cpp_function make(PM pm, M m) { + return cpp_function([pm](const T &c) -> const D & { return c.*pm; }, m); + } }; template @@ -1390,19 +1392,23 @@ template struct is_std_shared_ptr> : std::true_type {}; template -struct getter_cpp_function::value && - detail::type_uses_smart_holder_type_caster::value && - !is_std_shared_ptr::value>> { +struct getter_cpp_function< + T, + D, + detail::enable_if_t::value + && detail::type_uses_smart_holder_type_caster::value + && !is_std_shared_ptr::value>> { template static cpp_function make(PM pm, M m) { - return cpp_function([pm](const std::shared_ptr &c_sp) -> std::shared_ptr{ - const T &c = *c_sp.get(); - D &d = const_cast(c.*pm); - // Emulating PyCLIF approach: - // https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233 - return std::shared_ptr(c_sp, &d); - }, - m); + return cpp_function( + [pm](const std::shared_ptr &c_sp) -> std::shared_ptr { + const T &c = *c_sp.get(); + D &d = const_cast(c.*pm); + // Emulating PyCLIF approach: + // https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233 + return std::shared_ptr(c_sp, &d); + }, + m); } }; diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index 0c7d5b5d45..6a7975cec0 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -30,9 +30,9 @@ TEST_SUBMODULE(class_sh_property, m) { .def_readwrite("num", &Field::num) // ; - py::classh(m, "Outer") // - .def(py::init<>()) // - .def_readwrite("m_val", &Outer::m_val) // + py::classh(m, "Outer") // + .def(py::init<>()) // + .def_readwrite("m_val", &Outer::m_val) // .def_readwrite("m_sh_ptr", &Outer::m_sh_ptr) // ; From b3329aaf5fc38269469e54bcee4a20e1453c1269 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 3 Jan 2022 16:52:16 -0800 Subject: [PATCH 10/39] Also using getter_cpp_function for def_property_readonly. --- include/pybind11/pybind11.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 94a9e363af..ebec4762c3 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1624,8 +1624,11 @@ class class_ : public detail::generic_type { template class_ &def_readonly(const char *name, const D C::*pm, const Extra& ...extra) { static_assert(std::is_same::value || std::is_base_of::value, "def_readonly() requires a class member (or base class member)"); - cpp_function fget([pm](const type &c) -> const D &{ return c.*pm; }, is_method(*this)); - def_property_readonly(name, fget, return_value_policy::reference_internal, extra...); + def_property_readonly( + name, + getter_cpp_function::make(pm, is_method(*this)), + return_value_policy::reference_internal, + extra...); return *this; } From bf05ce8b43a65325f5eb9113cfa20098c00587a2 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 4 Jan 2022 13:46:17 -0800 Subject: [PATCH 11/39] 1. Resolving clang-tidy error (redundant `.get()`). 2. It turns out the `const_cast` is not needed. --- include/pybind11/pybind11.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index ebec4762c3..b8f3ad3b19 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1402,8 +1402,7 @@ struct getter_cpp_function< static cpp_function make(PM pm, M m) { return cpp_function( [pm](const std::shared_ptr &c_sp) -> std::shared_ptr { - const T &c = *c_sp.get(); - D &d = const_cast(c.*pm); + D &d = (*c_sp).*pm; // Emulating PyCLIF approach: // https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233 return std::shared_ptr(c_sp, &d); From 6ec3be4e58c20e31267c3dd5e137d4dbf4da7724 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 4 Jan 2022 13:57:47 -0800 Subject: [PATCH 12/39] Streamlining to `&(c_sp.get()->*pm)` inline. --- include/pybind11/pybind11.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index b8f3ad3b19..94c721a475 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1402,10 +1402,9 @@ struct getter_cpp_function< static cpp_function make(PM pm, M m) { return cpp_function( [pm](const std::shared_ptr &c_sp) -> std::shared_ptr { - D &d = (*c_sp).*pm; // Emulating PyCLIF approach: // https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233 - return std::shared_ptr(c_sp, &d); + return std::shared_ptr(c_sp, &(c_sp.get()->*pm)); }, m); } From fefb2426513fea328737cac793fe99c06a0c8afb Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 4 Jan 2022 15:07:24 -0800 Subject: [PATCH 13/39] Replacing `M m` with `const handle &hdl` for improved readability. --- include/pybind11/pybind11.h | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 94c721a475..4769ffdc38 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1379,10 +1379,10 @@ using default_holder_type = smart_holder; #endif template -struct getter_cpp_function { - template - static cpp_function make(PM pm, M m) { - return cpp_function([pm](const T &c) -> const D & { return c.*pm; }, m); +struct member_getter_cpp_function { + template + static cpp_function make(PM pm, const handle &hdl) { + return cpp_function([pm](const T &c) -> const D & { return c.*pm; }, is_method(hdl)); } }; @@ -1392,21 +1392,21 @@ template struct is_std_shared_ptr> : std::true_type {}; template -struct getter_cpp_function< +struct member_getter_cpp_function< T, D, detail::enable_if_t::value && detail::type_uses_smart_holder_type_caster::value && !is_std_shared_ptr::value>> { - template - static cpp_function make(PM pm, M m) { + template + static cpp_function make(PM pm, const handle &hdl) { return cpp_function( [pm](const std::shared_ptr &c_sp) -> std::shared_ptr { // Emulating PyCLIF approach: // https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233 return std::shared_ptr(c_sp, &(c_sp.get()->*pm)); }, - m); + is_method(hdl)); } }; @@ -1612,7 +1612,7 @@ class class_ : public detail::generic_type { static_assert(std::is_same::value || std::is_base_of::value, "def_readwrite() requires a class member (or base class member)"); def_property( name, - getter_cpp_function::make(pm, is_method(*this)), + member_getter_cpp_function::make(pm, *this), cpp_function([pm](type &c, const D &value) { c.*pm = value; }, is_method(*this)), return_value_policy::reference_internal, extra...); @@ -1624,7 +1624,7 @@ class class_ : public detail::generic_type { static_assert(std::is_same::value || std::is_base_of::value, "def_readonly() requires a class member (or base class member)"); def_property_readonly( name, - getter_cpp_function::make(pm, is_method(*this)), + member_getter_cpp_function::make(pm, *this), return_value_policy::reference_internal, extra...); return *this; From 776816ef69a083413bdbb6125e1a5e72872f0705 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 4 Jan 2022 16:27:41 -0800 Subject: [PATCH 14/39] Adding some m_mptr, m_uqmp, m_shmp code (highly incomplete, but this builds & tests). --- include/pybind11/pybind11.h | 10 ++++++++-- tests/test_class_sh_property.cpp | 16 ++++++++++------ tests/test_class_sh_property.py | 10 +++++----- 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 4769ffdc38..35d390a967 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1386,9 +1386,14 @@ struct member_getter_cpp_function { } }; -template +template +struct is_std_unique_ptr : std::false_type {}; +template +struct is_std_unique_ptr> : std::true_type {}; + +template struct is_std_shared_ptr : std::false_type {}; -template +template struct is_std_shared_ptr> : std::true_type {}; template @@ -1397,6 +1402,7 @@ struct member_getter_cpp_function< D, detail::enable_if_t::value && detail::type_uses_smart_holder_type_caster::value + && !is_std_unique_ptr::value // && !is_std_shared_ptr::value>> { template static cpp_function make(PM pm, const handle &hdl) { diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index 6a7975cec0..fb64877137 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -11,8 +11,10 @@ struct Field { }; struct Outer { - Field m_val; - std::shared_ptr m_sh_ptr; + Field m_valu; + Field *m_mptr; + std::unique_ptr m_uqmp; + std::shared_ptr m_shmp; }; inline void DisownOuter(std::unique_ptr) {} @@ -30,10 +32,12 @@ TEST_SUBMODULE(class_sh_property, m) { .def_readwrite("num", &Field::num) // ; - py::classh(m, "Outer") // - .def(py::init<>()) // - .def_readwrite("m_val", &Outer::m_val) // - .def_readwrite("m_sh_ptr", &Outer::m_sh_ptr) // + py::classh(m, "Outer") // + .def(py::init<>()) // + .def_readwrite("m_valu", &Outer::m_valu) // + // .def_readwrite("m_mptr", &Outer::m_mptr) // + // .def_readwrite("m_uqmp", &Outer::m_uqmp) // + .def_readwrite("m_shmp", &Outer::m_shmp) // ; m.def("DisownOuter", DisownOuter); diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index 76765012b9..5d71bbad99 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -10,7 +10,7 @@ def test_field_getter(msg): # Reduced from PyCLIF test: # https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/testing/python/nested_fields_test.py#L56 outer = m.Outer() - field = outer.m_val + field = outer.m_valu assert field.num == -99 with pytest.raises(ValueError) as excinfo: m.DisownOuter(outer) @@ -18,7 +18,7 @@ def test_field_getter(msg): del field m.DisownOuter(outer) with pytest.raises(ValueError) as excinfo: - outer.m_val + outer.m_valu assert ( msg(excinfo.value) == "Missing value for wrapped C++ type: Python instance was disowned." @@ -27,8 +27,8 @@ def test_field_getter(msg): def test_field_setter(msg): outer = m.Outer() - assert outer.m_val.num == -99 + assert outer.m_valu.num == -99 field = m.Field() field.num = 35 - outer.m_val = field - assert outer.m_val.num == 35 + outer.m_valu = field + assert outer.m_valu.num == 35 From 97e7d8d510f38f184d223a407dc03470d601a13e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 4 Jan 2022 17:58:24 -0800 Subject: [PATCH 15/39] Experiment: m_uqmp_disown, m_uqmp distinction; test_uqmp --- tests/test_class_sh_property.cpp | 22 +++++++++++++++++++--- tests/test_class_sh_property.py | 21 +++++++++++++++++++-- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index fb64877137..ee5ddd13c1 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -12,7 +12,7 @@ struct Field { struct Outer { Field m_valu; - Field *m_mptr; + Field *m_mptr = nullptr; std::unique_ptr m_uqmp; std::shared_ptr m_shmp; }; @@ -35,8 +35,24 @@ TEST_SUBMODULE(class_sh_property, m) { py::classh(m, "Outer") // .def(py::init<>()) // .def_readwrite("m_valu", &Outer::m_valu) // - // .def_readwrite("m_mptr", &Outer::m_mptr) // - // .def_readwrite("m_uqmp", &Outer::m_uqmp) // + .def_property_readonly( // + "m_mptr", + [](const std::shared_ptr &self) { + return std::shared_ptr(self, self->m_mptr); + }) + .def_property_readonly( // + "m_uqmp", + [](const std::shared_ptr &self) { + return std::shared_ptr(self, self->m_uqmp.get()); + }) + .def_property( // + "m_uqmp_disown", + [](const std::shared_ptr &self) { + return std::unique_ptr(std::move(self->m_uqmp)); + }, + [](Outer &self, std::unique_ptr uqmp) { + self.m_uqmp = std::move(uqmp); // + }) .def_readwrite("m_shmp", &Outer::m_shmp) // ; diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index 5d71bbad99..8a0996a7b7 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -6,7 +6,7 @@ @pytest.mark.xfail("env.PYPY", reason="gc after `del field` is apparently deferred") -def test_field_getter(msg): +def test_valu_getter(msg): # Reduced from PyCLIF test: # https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/testing/python/nested_fields_test.py#L56 outer = m.Outer() @@ -25,10 +25,27 @@ def test_field_getter(msg): ) -def test_field_setter(msg): +def test_valu_setter(): outer = m.Outer() assert outer.m_valu.num == -99 field = m.Field() field.num = 35 outer.m_valu = field assert outer.m_valu.num == 35 + + +def test_uqmp(msg): + outer = m.Outer() + assert outer.m_uqmp is None + field = m.Field() + field.num = 39 + outer.m_uqmp_disown = field + with pytest.raises(ValueError) as excinfo: + field.num + assert ( + msg(excinfo.value) + == "Missing value for wrapped C++ type: Python instance was disowned." + ) + field_getter = outer.m_uqmp + assert outer.m_uqmp.num == 39 + assert field_getter.num == 39 From bcda516d30bb36453abf95929545b4ec491afb4f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 5 Jan 2022 16:41:17 -0800 Subject: [PATCH 16/39] test_mptr with unique_ptr-as-holder and smart_holder --- tests/test_class_sh_property.cpp | 23 +++++++++++++++++++++-- tests/test_class_sh_property.py | 22 ++++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index ee5ddd13c1..e9b8fc911c 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -6,6 +6,14 @@ namespace test_class_sh_property { +struct ClassicField { + int num = -88; +}; + +struct ClassicOuter { + ClassicField *m_mptr = nullptr; +}; + struct Field { int num = -99; }; @@ -27,6 +35,16 @@ PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_property::Outer) TEST_SUBMODULE(class_sh_property, m) { using namespace test_class_sh_property; + py::class_>(m, "ClassicField") + .def(py::init<>()) // + .def_readwrite("num", &ClassicField::num) // + ; + + py::class_>(m, "ClassicOuter") + .def(py::init<>()) // + .def_readwrite("m_mptr", &ClassicOuter::m_mptr) // + ; + py::classh(m, "Field") // .def(py::init<>()) // .def_readwrite("num", &Field::num) // @@ -35,11 +53,12 @@ TEST_SUBMODULE(class_sh_property, m) { py::classh(m, "Outer") // .def(py::init<>()) // .def_readwrite("m_valu", &Outer::m_valu) // - .def_property_readonly( // + .def_property( // "m_mptr", [](const std::shared_ptr &self) { return std::shared_ptr(self, self->m_mptr); - }) + }, + [](Outer &self, Field *mptr) { self.m_mptr = mptr; }) .def_property_readonly( // "m_uqmp", [](const std::shared_ptr &self) { diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index 8a0996a7b7..d0e80046b2 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -34,6 +34,28 @@ def test_valu_setter(): assert outer.m_valu.num == 35 +@pytest.mark.parametrize( + "field_type, num_default, outer_type", + [ + (m.ClassicField, -88, m.ClassicOuter), + (m.Field, -99, m.Outer), + ], +) +def test_mptr(field_type, num_default, outer_type): + outer = outer_type() + assert outer.m_mptr is None + field = field_type() + assert field.num == num_default + outer.m_mptr = field + assert outer.m_mptr.num == num_default + field.num = 76 + assert outer.m_mptr.num == 76 + # Change to -88 or -99 to demonstrate Undefined Behavior (dangling pointer). + if num_default == 88: + del field + assert outer.m_mptr.num == 76 + + def test_uqmp(msg): outer = m.Outer() assert outer.m_uqmp is None From 2cb0e31a33b146d5f684ac23d9affbabcf93634a Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 5 Jan 2022 16:52:35 -0800 Subject: [PATCH 17/39] Adding test_shmp --- tests/test_class_sh_property.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index d0e80046b2..68872a9312 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -71,3 +71,16 @@ def test_uqmp(msg): field_getter = outer.m_uqmp assert outer.m_uqmp.num == 39 assert field_getter.num == 39 + + +def test_shmp(): + outer = m.Outer() + assert outer.m_shmp is None + field = m.Field() + field.num = 43 + outer.m_shmp = field + assert outer.m_shmp.num == 43 + outer.m_shmp.num = 57 + assert field.num == 57 + del field + assert outer.m_shmp.num == 57 From a2e248827b9d6d37262d78e2d9524e3bf010a4d1 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 5 Jan 2022 18:04:18 -0800 Subject: [PATCH 18/39] Adding const varieties (cptr, uqcp, shcp). uqmp & uqcp need work. --- tests/test_class_sh_property.cpp | 30 ++++++++++++++++++ tests/test_class_sh_property.py | 54 +++++++++++++++++++++----------- 2 files changed, 65 insertions(+), 19 deletions(-) diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index e9b8fc911c..5aea8d3083 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -12,6 +12,7 @@ struct ClassicField { struct ClassicOuter { ClassicField *m_mptr = nullptr; + const ClassicField *m_cptr = nullptr; }; struct Field { @@ -21,14 +22,22 @@ struct Field { struct Outer { Field m_valu; Field *m_mptr = nullptr; + const Field *m_cptr = nullptr; std::unique_ptr m_uqmp; + std::unique_ptr m_uqcp; std::shared_ptr m_shmp; + std::shared_ptr m_shcp; }; inline void DisownOuter(std::unique_ptr) {} } // namespace test_class_sh_property +PYBIND11_TYPE_CASTER_BASE_HOLDER(test_class_sh_property::ClassicField, + std::unique_ptr) +PYBIND11_TYPE_CASTER_BASE_HOLDER(test_class_sh_property::ClassicOuter, + std::unique_ptr) + PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_property::Field) PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_property::Outer) @@ -43,6 +52,7 @@ TEST_SUBMODULE(class_sh_property, m) { py::class_>(m, "ClassicOuter") .def(py::init<>()) // .def_readwrite("m_mptr", &ClassicOuter::m_mptr) // + .def_readwrite("m_cptr", &ClassicOuter::m_cptr) // ; py::classh(m, "Field") // @@ -59,6 +69,12 @@ TEST_SUBMODULE(class_sh_property, m) { return std::shared_ptr(self, self->m_mptr); }, [](Outer &self, Field *mptr) { self.m_mptr = mptr; }) + .def_property( // + "m_cptr", + [](const std::shared_ptr &self) { + return std::shared_ptr(self, self->m_cptr); + }, + [](Outer &self, const Field *cptr) { self.m_cptr = cptr; }) .def_property_readonly( // "m_uqmp", [](const std::shared_ptr &self) { @@ -72,7 +88,21 @@ TEST_SUBMODULE(class_sh_property, m) { [](Outer &self, std::unique_ptr uqmp) { self.m_uqmp = std::move(uqmp); // }) + .def_property_readonly( // + "m_uqcp", + [](const std::shared_ptr &self) { + return std::shared_ptr(self, self->m_uqcp.get()); + }) + .def_property( // + "m_uqcp_disown", + [](const std::shared_ptr &self) { + return std::unique_ptr(std::move(self->m_uqcp)); + }, + [](Outer &self, std::unique_ptr uqcp) { + self.m_uqcp = std::move(uqcp); // + }) .def_readwrite("m_shmp", &Outer::m_shmp) // + .def_readwrite("m_shcp", &Outer::m_shcp) // ; m.def("DisownOuter", DisownOuter); diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index 68872a9312..4dc4cbdcef 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -41,46 +41,62 @@ def test_valu_setter(): (m.Field, -99, m.Outer), ], ) -def test_mptr(field_type, num_default, outer_type): +@pytest.mark.parametrize("m_attr", ("m_mptr", "m_cptr")) +def test_ptr(field_type, num_default, outer_type, m_attr): outer = outer_type() - assert outer.m_mptr is None + assert getattr(outer, m_attr) is None field = field_type() assert field.num == num_default - outer.m_mptr = field - assert outer.m_mptr.num == num_default + setattr(outer, m_attr, field) + assert getattr(outer, m_attr).num == num_default field.num = 76 - assert outer.m_mptr.num == 76 + assert getattr(outer, m_attr).num == 76 # Change to -88 or -99 to demonstrate Undefined Behavior (dangling pointer). - if num_default == 88: + if num_default == 88 and m_attr == "m_mptr": del field - assert outer.m_mptr.num == 76 + assert getattr(outer, m_attr).num == 76 -def test_uqmp(msg): +@pytest.mark.parametrize("m_attr", ("m_uqmp", "m_uqcp")) +def test_uqp(m_attr, msg): + m_attr_disown = m_attr + "_disown" outer = m.Outer() - assert outer.m_uqmp is None + assert getattr(outer, m_attr) is None + assert getattr(outer, m_attr_disown) is None field = m.Field() field.num = 39 - outer.m_uqmp_disown = field + setattr(outer, m_attr_disown, field) with pytest.raises(ValueError) as excinfo: field.num assert ( msg(excinfo.value) == "Missing value for wrapped C++ type: Python instance was disowned." ) - field_getter = outer.m_uqmp - assert outer.m_uqmp.num == 39 - assert field_getter.num == 39 + field_co_own = getattr(outer, m_attr) + assert getattr(outer, m_attr).num == 39 + assert field_co_own.num == 39 + # TODO: needs work. + # with pytest.raises(RuntimeError) as excinfo: + # getattr(outer, m_attr_disown) + # assert ( + # msg(excinfo.value) + # == "Invalid unique_ptr: another instance owns this pointer already." + # ) + del field_co_own + field_excl_own = getattr(outer, m_attr_disown) + assert getattr(outer, m_attr) is None + assert field_excl_own.num == 39 -def test_shmp(): +@pytest.mark.parametrize("m_attr", ("m_shmp", "m_shcp")) +def test_shp(m_attr): outer = m.Outer() - assert outer.m_shmp is None + assert getattr(outer, m_attr) is None field = m.Field() field.num = 43 - outer.m_shmp = field - assert outer.m_shmp.num == 43 - outer.m_shmp.num = 57 + setattr(outer, m_attr, field) + assert getattr(outer, m_attr).num == 43 + getattr(outer, m_attr).num = 57 assert field.num == 57 del field - assert outer.m_shmp.num == 57 + assert getattr(outer, m_attr).num == 57 From 5c760bea118536836a0d45b9714d5a02263b9f31 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 6 Jan 2022 10:07:13 -0800 Subject: [PATCH 19/39] Adding another PyPy xfail. --- tests/test_class_sh_property.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index 4dc4cbdcef..4deb99c6df 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -57,6 +57,9 @@ def test_ptr(field_type, num_default, outer_type, m_attr): assert getattr(outer, m_attr).num == 76 +@pytest.mark.xfail( + "env.PYPY", reason="gc after `del field_co_own` is apparently deferred" +) @pytest.mark.parametrize("m_attr", ("m_uqmp", "m_uqcp")) def test_uqp(m_attr, msg): m_attr_disown = m_attr + "_disown" From f06d6fafb13b6705bef86c98b2fa13fa515d3959 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 10 Jan 2022 18:37:50 -0800 Subject: [PATCH 20/39] unique_ptr_field_proxy_poc (proof of concept) --- tests/test_class_sh_property.cpp | 10 ---- tests/test_class_sh_property.py | 84 +++++++++++++++++++++++--------- 2 files changed, 62 insertions(+), 32 deletions(-) diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index 5aea8d3083..eeca8535bb 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -75,11 +75,6 @@ TEST_SUBMODULE(class_sh_property, m) { return std::shared_ptr(self, self->m_cptr); }, [](Outer &self, const Field *cptr) { self.m_cptr = cptr; }) - .def_property_readonly( // - "m_uqmp", - [](const std::shared_ptr &self) { - return std::shared_ptr(self, self->m_uqmp.get()); - }) .def_property( // "m_uqmp_disown", [](const std::shared_ptr &self) { @@ -88,11 +83,6 @@ TEST_SUBMODULE(class_sh_property, m) { [](Outer &self, std::unique_ptr uqmp) { self.m_uqmp = std::move(uqmp); // }) - .def_property_readonly( // - "m_uqcp", - [](const std::shared_ptr &self) { - return std::shared_ptr(self, self->m_uqcp.get()); - }) .def_property( // "m_uqcp_disown", [](const std::shared_ptr &self) { diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index 4deb99c6df..ea9ce1a1e3 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -60,35 +60,75 @@ def test_ptr(field_type, num_default, outer_type, m_attr): @pytest.mark.xfail( "env.PYPY", reason="gc after `del field_co_own` is apparently deferred" ) -@pytest.mark.parametrize("m_attr", ("m_uqmp", "m_uqcp")) -def test_uqp(m_attr, msg): - m_attr_disown = m_attr + "_disown" +@pytest.mark.parametrize("m_attr_disown", ("m_uqmp_disown", "m_uqcp_disown")) +def test_uqp(m_attr_disown, msg): outer = m.Outer() - assert getattr(outer, m_attr) is None assert getattr(outer, m_attr_disown) is None - field = m.Field() - field.num = 39 - setattr(outer, m_attr_disown, field) + field_orig = m.Field() + field_orig.num = 39 + setattr(outer, m_attr_disown, field_orig) with pytest.raises(ValueError) as excinfo: - field.num + field_orig.num assert ( msg(excinfo.value) == "Missing value for wrapped C++ type: Python instance was disowned." ) - field_co_own = getattr(outer, m_attr) - assert getattr(outer, m_attr).num == 39 - assert field_co_own.num == 39 - # TODO: needs work. - # with pytest.raises(RuntimeError) as excinfo: - # getattr(outer, m_attr_disown) - # assert ( - # msg(excinfo.value) - # == "Invalid unique_ptr: another instance owns this pointer already." - # ) - del field_co_own - field_excl_own = getattr(outer, m_attr_disown) - assert getattr(outer, m_attr) is None - assert field_excl_own.num == 39 + field_retr1 = getattr(outer, m_attr_disown) + assert getattr(outer, m_attr_disown) is None + assert field_retr1.num == 39 + field_retr1.num = 93 + setattr(outer, m_attr_disown, field_retr1) + with pytest.raises(ValueError): + field_retr1.num + field_retr2 = getattr(outer, m_attr_disown) + assert field_retr2.num == 93 + + +def _dereference(proxy, xxxattr, *args, **kwargs): + obj = object.__getattribute__(proxy, "__obj") + field_name = object.__getattribute__(proxy, "__field_name") + field = getattr(obj, field_name) + assert field is not None + try: + return xxxattr(field, *args, **kwargs) + finally: + setattr(obj, field_name, field) + + +class unique_ptr_field_proxy_poc(object): # noqa: N801 + def __init__(self, obj, field_name): + object.__setattr__(self, "__obj", obj) + object.__setattr__(self, "__field_name", field_name) + + def __getattr__(self, *args, **kwargs): + return _dereference(self, getattr, *args, **kwargs) + + def __setattr__(self, *args, **kwargs): + return _dereference(self, setattr, *args, **kwargs) + + def __delattr__(self, *args, **kwargs): + return _dereference(self, delattr, *args, **kwargs) + + +@pytest.mark.parametrize("m_attr_disown", ("m_uqmp_disown", "m_uqcp_disown")) +def test_unique_ptr_field_proxy_poc(m_attr_disown, msg): + outer = m.Outer() + field_orig = m.Field() + field_orig.num = 45 + setattr(outer, m_attr_disown, field_orig) + field_proxy = unique_ptr_field_proxy_poc(outer, m_attr_disown) + assert field_proxy.num == 45 + assert field_proxy.num == 45 + with pytest.raises(AttributeError): + field_proxy.xyz + assert field_proxy.num == 45 + field_proxy.num = 82 + assert field_proxy.num == 82 + field_proxy = unique_ptr_field_proxy_poc(outer, m_attr_disown) + assert field_proxy.num == 82 + with pytest.raises(AttributeError): + del field_proxy.num + assert field_proxy.num == 82 @pytest.mark.parametrize("m_attr", ("m_shmp", "m_shcp")) From ae1c898d8f717dfbf796e4fa07ccd3a84dacb960 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 12 Jan 2022 11:05:57 -0800 Subject: [PATCH 21/39] Adjustments related to PR #3590 after `git rebase -X theirs smart_holder` --- tests/CMakeLists.txt | 111 ++++++++++++++++++++++--------------------- 1 file changed, 56 insertions(+), 55 deletions(-) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 90485300e2..a23cb2d44a 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -115,61 +115,62 @@ endif() # Any test that has no extension is both .py and .cpp, so 'foo' will add 'foo.cpp' and 'foo.py'. # Any test that has an extension is exclusively that and handled as such. set(PYBIND11_TEST_FILES - test_async.cpp - test_buffers.cpp - test_builtin_casters.cpp - test_call_policies.cpp - test_callbacks.cpp - test_chrono.cpp - test_class.cpp - test_class_sh_basic.cpp - test_class_sh_disowning.cpp - test_class_sh_disowning_mi.cpp - test_class_sh_factory_constructors.cpp - test_class_sh_inheritance.cpp - test_class_sh_property.cpp - test_class_sh_shared_ptr_copy_move.cpp - test_class_sh_trampoline_basic.cpp - test_class_sh_trampoline_self_life_support.cpp - test_class_sh_trampoline_shared_from_this.cpp - test_class_sh_trampoline_shared_ptr_cpp_arg.cpp - test_class_sh_trampoline_unique_ptr.cpp - test_class_sh_unique_ptr_member.cpp - test_class_sh_virtual_py_cpp_mix.cpp - test_classh_mock.cpp - test_const_name.cpp - test_constants_and_functions.cpp - test_copy_move.cpp - test_custom_type_casters.cpp - test_custom_type_setup.cpp - test_docstring_options.cpp - test_eigen.cpp - test_enum.cpp - test_eval.cpp - test_exceptions.cpp - test_factory_constructors.cpp - test_gil_scoped.cpp - test_iostream.cpp - test_kwargs_and_defaults.cpp - test_local_bindings.cpp - test_methods_and_attributes.cpp - test_modules.cpp - test_multiple_inheritance.cpp - test_numpy_array.cpp - test_numpy_dtypes.cpp - test_numpy_vectorize.cpp - test_opaque_types.cpp - test_operator_overloading.cpp - test_pickling.cpp - test_pytypes.cpp - test_sequences_and_iterators.cpp - test_smart_ptr.cpp - test_stl.cpp - test_stl_binders.cpp - test_tagbased_polymorphic.cpp - test_thread.cpp - test_union.cpp - test_virtual_functions.cpp) + test_async + test_buffers + test_builtin_casters + test_call_policies + test_callbacks + test_chrono + test_class + test_class_sh_basic + test_class_sh_disowning + test_class_sh_disowning_mi + test_class_sh_factory_constructors + test_class_sh_inheritance + test_class_sh_module_local.py + test_class_sh_property + test_class_sh_shared_ptr_copy_move + test_class_sh_trampoline_basic + test_class_sh_trampoline_self_life_support + test_class_sh_trampoline_shared_from_this + test_class_sh_trampoline_shared_ptr_cpp_arg + test_class_sh_trampoline_unique_ptr + test_class_sh_unique_ptr_member + test_class_sh_virtual_py_cpp_mix + test_classh_mock + test_const_name + test_constants_and_functions + test_copy_move + test_custom_type_casters + test_custom_type_setup + test_docstring_options + test_eigen + test_enum + test_eval + test_exceptions + test_factory_constructors + test_gil_scoped + test_iostream + test_kwargs_and_defaults + test_local_bindings + test_methods_and_attributes + test_modules + test_multiple_inheritance + test_numpy_array + test_numpy_dtypes + test_numpy_vectorize + test_opaque_types + test_operator_overloading + test_pickling + test_pytypes + test_sequences_and_iterators + test_smart_ptr + test_stl + test_stl_binders + test_tagbased_polymorphic + test_thread + test_union + test_virtual_functions) # Invoking cmake with something like: # cmake -DPYBIND11_TEST_OVERRIDE="test_callbacks.cpp;test_pickling.cpp" .. From 036fa048db5dd68d94954b7433887efd77e09171 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 12 Jan 2022 11:38:59 -0800 Subject: [PATCH 22/39] Removing PyPy xfail for test_uqp (forgot before). --- tests/test_class_sh_property.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index ea9ce1a1e3..3d232c84b7 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -57,9 +57,6 @@ def test_ptr(field_type, num_default, outer_type, m_attr): assert getattr(outer, m_attr).num == 76 -@pytest.mark.xfail( - "env.PYPY", reason="gc after `del field_co_own` is apparently deferred" -) @pytest.mark.parametrize("m_attr_disown", ("m_uqmp_disown", "m_uqcp_disown")) def test_uqp(m_attr_disown, msg): outer = m.Outer() From 35f28789dce4bd2bd6e62142e7cfb5a5e74870d3 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 20 Jan 2022 11:57:49 -0800 Subject: [PATCH 23/39] Trivial changes to set the stage for working on def_readwrite specializations. --- tests/test_class_sh_property.cpp | 12 ++++++++---- tests/test_class_sh_property.py | 13 +++++++------ 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index eeca8535bb..c29a770f5d 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -64,19 +64,21 @@ TEST_SUBMODULE(class_sh_property, m) { .def(py::init<>()) // .def_readwrite("m_valu", &Outer::m_valu) // .def_property( // - "m_mptr", + "m_mptr_property", [](const std::shared_ptr &self) { return std::shared_ptr(self, self->m_mptr); }, [](Outer &self, Field *mptr) { self.m_mptr = mptr; }) .def_property( // - "m_cptr", + "m_cptr_property", [](const std::shared_ptr &self) { return std::shared_ptr(self, self->m_cptr); }, [](Outer &self, const Field *cptr) { self.m_cptr = cptr; }) + // .def_readwrite("m_mptr", &Outer::m_mptr) + // .def_readwrite("m_cptr", &Outer::m_cptr) .def_property( // - "m_uqmp_disown", + "m_uqmp_property", [](const std::shared_ptr &self) { return std::unique_ptr(std::move(self->m_uqmp)); }, @@ -84,13 +86,15 @@ TEST_SUBMODULE(class_sh_property, m) { self.m_uqmp = std::move(uqmp); // }) .def_property( // - "m_uqcp_disown", + "m_uqcp_property", [](const std::shared_ptr &self) { return std::unique_ptr(std::move(self->m_uqcp)); }, [](Outer &self, std::unique_ptr uqcp) { self.m_uqcp = std::move(uqcp); // }) + // .def_readwrite("m_uqmp", &Outer::m_uqmp) // + // .def_readwrite("m_uqcp", &Outer::m_uqcp) // .def_readwrite("m_shmp", &Outer::m_shmp) // .def_readwrite("m_shcp", &Outer::m_shcp) // ; diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index 3d232c84b7..b6808e601f 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -35,14 +35,15 @@ def test_valu_setter(): @pytest.mark.parametrize( - "field_type, num_default, outer_type", + "field_type, num_default, outer_type, attr_suffix", [ - (m.ClassicField, -88, m.ClassicOuter), - (m.Field, -99, m.Outer), + (m.ClassicField, -88, m.ClassicOuter, ""), + (m.Field, -99, m.Outer, "_property"), ], ) @pytest.mark.parametrize("m_attr", ("m_mptr", "m_cptr")) -def test_ptr(field_type, num_default, outer_type, m_attr): +def test_ptr(field_type, num_default, outer_type, attr_suffix, m_attr): + m_attr += attr_suffix outer = outer_type() assert getattr(outer, m_attr) is None field = field_type() @@ -57,7 +58,7 @@ def test_ptr(field_type, num_default, outer_type, m_attr): assert getattr(outer, m_attr).num == 76 -@pytest.mark.parametrize("m_attr_disown", ("m_uqmp_disown", "m_uqcp_disown")) +@pytest.mark.parametrize("m_attr_disown", ("m_uqmp_property", "m_uqcp_property")) def test_uqp(m_attr_disown, msg): outer = m.Outer() assert getattr(outer, m_attr_disown) is None @@ -107,7 +108,7 @@ def __delattr__(self, *args, **kwargs): return _dereference(self, delattr, *args, **kwargs) -@pytest.mark.parametrize("m_attr_disown", ("m_uqmp_disown", "m_uqcp_disown")) +@pytest.mark.parametrize("m_attr_disown", ("m_uqmp_property", "m_uqcp_property")) def test_unique_ptr_field_proxy_poc(m_attr_disown, msg): outer = m.Outer() field_orig = m.Field() From c73604a652d971ad22cf77aa8fcad48fabaff8f0 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 20 Jan 2022 12:13:25 -0800 Subject: [PATCH 24/39] Introducing struct member_setter_cpp_function. --- include/pybind11/pybind11.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 35d390a967..adbc5d5550 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1386,6 +1386,14 @@ struct member_getter_cpp_function { } }; +template +struct member_setter_cpp_function { + template + static cpp_function make(PM pm, const handle &hdl) { + return cpp_function([pm](T &c, const D &value) { c.*pm = value; }, is_method(hdl)); + } +}; + template struct is_std_unique_ptr : std::false_type {}; template @@ -1619,7 +1627,7 @@ class class_ : public detail::generic_type { def_property( name, member_getter_cpp_function::make(pm, *this), - cpp_function([pm](type &c, const D &value) { c.*pm = value; }, is_method(*this)), + member_setter_cpp_function::make(pm, *this), return_value_policy::reference_internal, extra...); return *this; From 3bcfa8abd7178721416c67ba0a02127e60d24b51 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 20 Jan 2022 15:18:36 -0800 Subject: [PATCH 25/39] Support for def_readonly, def_readwrite m_mptr, m_cptr. --- include/pybind11/pybind11.h | 36 ++++++++++++++++++++++++++++++++ tests/test_class_sh_property.cpp | 11 ++++++---- tests/test_class_sh_property.py | 4 ++-- 3 files changed, 45 insertions(+), 6 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index adbc5d5550..00a6a6fe6d 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1378,6 +1378,8 @@ using default_holder_type = smart_holder; #endif +// TODO: move new code into detail namespace. + template struct member_getter_cpp_function { template @@ -1410,6 +1412,40 @@ struct member_getter_cpp_function< D, detail::enable_if_t::value && detail::type_uses_smart_holder_type_caster::value + && std::is_pointer::value>> { + using drp = typename std::remove_pointer::type; + template + static cpp_function make(PM pm, const handle &hdl) { + return cpp_function( + [pm](const std::shared_ptr &c_sp) -> std::shared_ptr { + D ptr = (*c_sp).*pm; + return std::shared_ptr(c_sp, ptr); + }, + is_method(hdl)); + } +}; + +template +struct member_setter_cpp_function< + T, + D, + detail::enable_if_t::value + && detail::type_uses_smart_holder_type_caster::value + && std::is_pointer::value>> { + using drp = typename std::remove_pointer::type; + template + static cpp_function make(PM pm, const handle &hdl) { + return cpp_function([pm](T &c, D value) { c.*pm = value; }, is_method(hdl)); + } +}; + +template +struct member_getter_cpp_function< + T, + D, + detail::enable_if_t::value + && detail::type_uses_smart_holder_type_caster::value + && !std::is_pointer::value // && !is_std_unique_ptr::value // && !is_std_shared_ptr::value>> { template diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index c29a770f5d..14acd6d3d4 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -60,8 +60,9 @@ TEST_SUBMODULE(class_sh_property, m) { .def_readwrite("num", &Field::num) // ; - py::classh(m, "Outer") // - .def(py::init<>()) // + py::classh(m, "Outer") // + .def(py::init<>()) // + // .def_readonly("m_valu_readonly", &Outer::m_valu) // .def_readwrite("m_valu", &Outer::m_valu) // .def_property( // "m_mptr_property", @@ -75,8 +76,10 @@ TEST_SUBMODULE(class_sh_property, m) { return std::shared_ptr(self, self->m_cptr); }, [](Outer &self, const Field *cptr) { self.m_cptr = cptr; }) - // .def_readwrite("m_mptr", &Outer::m_mptr) - // .def_readwrite("m_cptr", &Outer::m_cptr) + .def_readonly("m_mptr_readonly", &Outer::m_mptr) + .def_readwrite("m_mptr", &Outer::m_mptr) + .def_readonly("m_cptr_readonly", &Outer::m_cptr) + .def_readwrite("m_cptr", &Outer::m_cptr) .def_property( // "m_uqmp_property", [](const std::shared_ptr &self) { diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index b6808e601f..035303a246 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -35,10 +35,10 @@ def test_valu_setter(): @pytest.mark.parametrize( - "field_type, num_default, outer_type, attr_suffix", + "field_type, num_default, outer_type, attr_suffix", # TODO: rm attr_suffix [ (m.ClassicField, -88, m.ClassicOuter, ""), - (m.Field, -99, m.Outer, "_property"), + (m.Field, -99, m.Outer, ""), ], ) @pytest.mark.parametrize("m_attr", ("m_mptr", "m_cptr")) From 93a43895a8345aed9727556187dee190309cb555 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 20 Jan 2022 15:55:45 -0800 Subject: [PATCH 26/39] Combining member_getter_cpp_function, struct member_setter_cpp_function pairs struct xetter_cpp_function with read, write. --- include/pybind11/pybind11.h | 41 ++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 00a6a6fe6d..e40710929f 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1381,17 +1381,14 @@ using default_holder_type = smart_holder; // TODO: move new code into detail namespace. template -struct member_getter_cpp_function { +struct xetter_cpp_function { template - static cpp_function make(PM pm, const handle &hdl) { + static cpp_function read(PM pm, const handle &hdl) { return cpp_function([pm](const T &c) -> const D & { return c.*pm; }, is_method(hdl)); } -}; -template -struct member_setter_cpp_function { template - static cpp_function make(PM pm, const handle &hdl) { + static cpp_function write(PM pm, const handle &hdl) { return cpp_function([pm](T &c, const D &value) { c.*pm = value; }, is_method(hdl)); } }; @@ -1407,15 +1404,17 @@ template struct is_std_shared_ptr> : std::true_type {}; template -struct member_getter_cpp_function< +struct xetter_cpp_function< T, D, detail::enable_if_t::value && detail::type_uses_smart_holder_type_caster::value && std::is_pointer::value>> { + using drp = typename std::remove_pointer::type; + template - static cpp_function make(PM pm, const handle &hdl) { + static cpp_function read(PM pm, const handle &hdl) { return cpp_function( [pm](const std::shared_ptr &c_sp) -> std::shared_ptr { D ptr = (*c_sp).*pm; @@ -1423,24 +1422,15 @@ struct member_getter_cpp_function< }, is_method(hdl)); } -}; -template -struct member_setter_cpp_function< - T, - D, - detail::enable_if_t::value - && detail::type_uses_smart_holder_type_caster::value - && std::is_pointer::value>> { - using drp = typename std::remove_pointer::type; template - static cpp_function make(PM pm, const handle &hdl) { + static cpp_function write(PM pm, const handle &hdl) { return cpp_function([pm](T &c, D value) { c.*pm = value; }, is_method(hdl)); } }; template -struct member_getter_cpp_function< +struct xetter_cpp_function< T, D, detail::enable_if_t::value @@ -1449,7 +1439,7 @@ struct member_getter_cpp_function< && !is_std_unique_ptr::value // && !is_std_shared_ptr::value>> { template - static cpp_function make(PM pm, const handle &hdl) { + static cpp_function read(PM pm, const handle &hdl) { return cpp_function( [pm](const std::shared_ptr &c_sp) -> std::shared_ptr { // Emulating PyCLIF approach: @@ -1458,6 +1448,11 @@ struct member_getter_cpp_function< }, is_method(hdl)); } + + template + static cpp_function write(PM pm, const handle &hdl) { + return cpp_function([pm](T &c, const D &value) { c.*pm = value; }, is_method(hdl)); + } }; // clang-format off @@ -1662,8 +1657,8 @@ class class_ : public detail::generic_type { static_assert(std::is_same::value || std::is_base_of::value, "def_readwrite() requires a class member (or base class member)"); def_property( name, - member_getter_cpp_function::make(pm, *this), - member_setter_cpp_function::make(pm, *this), + xetter_cpp_function::read(pm, *this), + xetter_cpp_function::write(pm, *this), return_value_policy::reference_internal, extra...); return *this; @@ -1674,7 +1669,7 @@ class class_ : public detail::generic_type { static_assert(std::is_same::value || std::is_base_of::value, "def_readonly() requires a class member (or base class member)"); def_property_readonly( name, - member_getter_cpp_function::make(pm, *this), + xetter_cpp_function::read(pm, *this), return_value_policy::reference_internal, extra...); return *this; From 43052f0bcbceafc7e9466bd3f0e88d00e0af6ee6 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 20 Jan 2022 16:13:22 -0800 Subject: [PATCH 27/39] Distinguishing between xetter_cpp_function::readonly, read, but read just forwards to readonly (to get started). --- include/pybind11/pybind11.h | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index e40710929f..ee5b92bc5c 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1383,10 +1383,15 @@ using default_holder_type = smart_holder; template struct xetter_cpp_function { template - static cpp_function read(PM pm, const handle &hdl) { + static cpp_function readonly(PM pm, const handle &hdl) { return cpp_function([pm](const T &c) -> const D & { return c.*pm; }, is_method(hdl)); } + template + static cpp_function read(PM pm, const handle &hdl) { + return readonly(pm, hdl); + } + template static cpp_function write(PM pm, const handle &hdl) { return cpp_function([pm](T &c, const D &value) { c.*pm = value; }, is_method(hdl)); @@ -1414,7 +1419,7 @@ struct xetter_cpp_function< using drp = typename std::remove_pointer::type; template - static cpp_function read(PM pm, const handle &hdl) { + static cpp_function readonly(PM pm, const handle &hdl) { return cpp_function( [pm](const std::shared_ptr &c_sp) -> std::shared_ptr { D ptr = (*c_sp).*pm; @@ -1423,6 +1428,11 @@ struct xetter_cpp_function< is_method(hdl)); } + template + static cpp_function read(PM pm, const handle &hdl) { + return readonly(pm, hdl); + } + template static cpp_function write(PM pm, const handle &hdl) { return cpp_function([pm](T &c, D value) { c.*pm = value; }, is_method(hdl)); @@ -1439,7 +1449,7 @@ struct xetter_cpp_function< && !is_std_unique_ptr::value // && !is_std_shared_ptr::value>> { template - static cpp_function read(PM pm, const handle &hdl) { + static cpp_function readonly(PM pm, const handle &hdl) { return cpp_function( [pm](const std::shared_ptr &c_sp) -> std::shared_ptr { // Emulating PyCLIF approach: @@ -1449,6 +1459,11 @@ struct xetter_cpp_function< is_method(hdl)); } + template + static cpp_function read(PM pm, const handle &hdl) { + return readonly(pm, hdl); + } + template static cpp_function write(PM pm, const handle &hdl) { return cpp_function([pm](T &c, const D &value) { c.*pm = value; }, is_method(hdl)); @@ -1669,7 +1684,7 @@ class class_ : public detail::generic_type { static_assert(std::is_same::value || std::is_base_of::value, "def_readonly() requires a class member (or base class member)"); def_property_readonly( name, - xetter_cpp_function::read(pm, *this), + xetter_cpp_function::readonly(pm, *this), return_value_policy::reference_internal, extra...); return *this; From 0c36cfd80e42077196d453ea35b7bedcd68ff660 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 20 Jan 2022 16:32:28 -0800 Subject: [PATCH 28/39] Special xetter_cpp_function::readonly to support m_valu_readonly. --- include/pybind11/pybind11.h | 15 ++++++++++----- tests/test_class_sh_property.cpp | 2 +- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index ee5b92bc5c..5fd15cbb1f 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1451,17 +1451,22 @@ struct xetter_cpp_function< template static cpp_function readonly(PM pm, const handle &hdl) { return cpp_function( - [pm](const std::shared_ptr &c_sp) -> std::shared_ptr { - // Emulating PyCLIF approach: - // https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233 - return std::shared_ptr(c_sp, &(c_sp.get()->*pm)); + [pm](const std::shared_ptr &c_sp) + -> std::shared_ptr::type> { + return std::shared_ptr::type>(c_sp, &(c_sp.get()->*pm)); }, is_method(hdl)); } template static cpp_function read(PM pm, const handle &hdl) { - return readonly(pm, hdl); + return cpp_function( + [pm](const std::shared_ptr &c_sp) -> std::shared_ptr { + // Emulating PyCLIF approach: + // https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233 + return std::shared_ptr(c_sp, &(c_sp.get()->*pm)); + }, + is_method(hdl)); } template diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index 14acd6d3d4..01dda46973 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -62,7 +62,7 @@ TEST_SUBMODULE(class_sh_property, m) { py::classh(m, "Outer") // .def(py::init<>()) // - // .def_readonly("m_valu_readonly", &Outer::m_valu) // + .def_readonly("m_valu_readonly", &Outer::m_valu) // .def_readwrite("m_valu", &Outer::m_valu) // .def_property( // "m_mptr_property", From 9b8dca04c6532e2677b7148691eeab495d8f26a5 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 20 Jan 2022 17:23:36 -0800 Subject: [PATCH 29/39] Making tests more complete, with cleanup. --- tests/test_class_sh_property.cpp | 50 +++++++++---------- tests/test_class_sh_property.py | 83 +++++++++++++++++++------------- 2 files changed, 73 insertions(+), 60 deletions(-) diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index 01dda46973..b5d815fc84 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -50,9 +50,11 @@ TEST_SUBMODULE(class_sh_property, m) { ; py::class_>(m, "ClassicOuter") - .def(py::init<>()) // - .def_readwrite("m_mptr", &ClassicOuter::m_mptr) // - .def_readwrite("m_cptr", &ClassicOuter::m_cptr) // + .def(py::init<>()) + .def_readonly("m_mptr_readonly", &ClassicOuter::m_mptr) + .def_readwrite("m_mptr_readwrite", &ClassicOuter::m_mptr) + .def_readwrite("m_cptr_readonly", &ClassicOuter::m_cptr) + .def_readwrite("m_cptr_readwrite", &ClassicOuter::m_cptr) // ; py::classh(m, "Field") // @@ -61,27 +63,18 @@ TEST_SUBMODULE(class_sh_property, m) { ; py::classh(m, "Outer") // - .def(py::init<>()) // - .def_readonly("m_valu_readonly", &Outer::m_valu) // - .def_readwrite("m_valu", &Outer::m_valu) // - .def_property( // - "m_mptr_property", - [](const std::shared_ptr &self) { - return std::shared_ptr(self, self->m_mptr); - }, - [](Outer &self, Field *mptr) { self.m_mptr = mptr; }) - .def_property( // - "m_cptr_property", - [](const std::shared_ptr &self) { - return std::shared_ptr(self, self->m_cptr); - }, - [](Outer &self, const Field *cptr) { self.m_cptr = cptr; }) + .def(py::init<>()) + + .def_readonly("m_valu_readonly", &Outer::m_valu) + .def_readwrite("m_valu_readwrite", &Outer::m_valu) + .def_readonly("m_mptr_readonly", &Outer::m_mptr) - .def_readwrite("m_mptr", &Outer::m_mptr) + .def_readwrite("m_mptr_readwrite", &Outer::m_mptr) .def_readonly("m_cptr_readonly", &Outer::m_cptr) - .def_readwrite("m_cptr", &Outer::m_cptr) + .def_readwrite("m_cptr_readwrite", &Outer::m_cptr) + .def_property( // - "m_uqmp_property", + "m_uqmp_readwrite", [](const std::shared_ptr &self) { return std::unique_ptr(std::move(self->m_uqmp)); }, @@ -89,17 +82,22 @@ TEST_SUBMODULE(class_sh_property, m) { self.m_uqmp = std::move(uqmp); // }) .def_property( // - "m_uqcp_property", + "m_uqcp_readwrite", [](const std::shared_ptr &self) { return std::unique_ptr(std::move(self->m_uqcp)); }, [](Outer &self, std::unique_ptr uqcp) { self.m_uqcp = std::move(uqcp); // }) - // .def_readwrite("m_uqmp", &Outer::m_uqmp) // - // .def_readwrite("m_uqcp", &Outer::m_uqcp) // - .def_readwrite("m_shmp", &Outer::m_shmp) // - .def_readwrite("m_shcp", &Outer::m_shcp) // + // .def_readonly("m_uqmp_readonly", &Outer::m_uqmp) // Compilation Error. + // .def_readwrite("m_uqmp_readwrite", &Outer::m_uqmp) + // .def_readonly("m_uqcp_readonly", &Outer::m_uqcp) // Compilation Error. + // .def_readwrite("m_uqcp_readwrite", &Outer::m_uqcp) + + .def_readwrite("m_shmp_readonly", &Outer::m_shmp) + .def_readwrite("m_shmp_readwrite", &Outer::m_shmp) + .def_readwrite("m_shcp_readonly", &Outer::m_shcp) + .def_readwrite("m_shcp_readwrite", &Outer::m_shcp) // ; m.def("DisownOuter", DisownOuter); diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index 035303a246..e055c44901 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -6,11 +6,12 @@ @pytest.mark.xfail("env.PYPY", reason="gc after `del field` is apparently deferred") -def test_valu_getter(msg): +@pytest.mark.parametrize("m_attr", ("m_valu_readonly", "m_valu_readwrite")) +def test_valu_getter(msg, m_attr): # Reduced from PyCLIF test: # https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/testing/python/nested_fields_test.py#L56 outer = m.Outer() - field = outer.m_valu + field = getattr(outer, m_attr) assert field.num == -99 with pytest.raises(ValueError) as excinfo: m.DisownOuter(outer) @@ -18,7 +19,7 @@ def test_valu_getter(msg): del field m.DisownOuter(outer) with pytest.raises(ValueError) as excinfo: - outer.m_valu + getattr(outer, m_attr) assert ( msg(excinfo.value) == "Missing value for wrapped C++ type: Python instance was disowned." @@ -27,58 +28,65 @@ def test_valu_getter(msg): def test_valu_setter(): outer = m.Outer() - assert outer.m_valu.num == -99 + assert outer.m_valu_readonly.num == -99 + assert outer.m_valu_readwrite.num == -99 field = m.Field() field.num = 35 - outer.m_valu = field - assert outer.m_valu.num == 35 + outer.m_valu_readwrite = field + assert outer.m_valu_readonly.num == 35 + assert outer.m_valu_readwrite.num == 35 @pytest.mark.parametrize( - "field_type, num_default, outer_type, attr_suffix", # TODO: rm attr_suffix + "field_type, num_default, outer_type", [ - (m.ClassicField, -88, m.ClassicOuter, ""), - (m.Field, -99, m.Outer, ""), + (m.ClassicField, -88, m.ClassicOuter), + (m.Field, -99, m.Outer), ], ) @pytest.mark.parametrize("m_attr", ("m_mptr", "m_cptr")) -def test_ptr(field_type, num_default, outer_type, attr_suffix, m_attr): - m_attr += attr_suffix +def test_ptr(field_type, num_default, outer_type, m_attr): + m_attr_readonly = m_attr + "_readonly" + m_attr_readwrite = m_attr + "_readwrite" outer = outer_type() - assert getattr(outer, m_attr) is None + assert getattr(outer, m_attr_readonly) is None + assert getattr(outer, m_attr_readwrite) is None field = field_type() assert field.num == num_default - setattr(outer, m_attr, field) - assert getattr(outer, m_attr).num == num_default + setattr(outer, m_attr_readwrite, field) + assert getattr(outer, m_attr_readonly).num == num_default + assert getattr(outer, m_attr_readwrite).num == num_default field.num = 76 - assert getattr(outer, m_attr).num == 76 + assert getattr(outer, m_attr_readonly).num == 76 + assert getattr(outer, m_attr_readwrite).num == 76 # Change to -88 or -99 to demonstrate Undefined Behavior (dangling pointer). if num_default == 88 and m_attr == "m_mptr": del field - assert getattr(outer, m_attr).num == 76 + assert getattr(outer, m_attr_readonly).num == 76 + assert getattr(outer, m_attr_readwrite).num == 76 -@pytest.mark.parametrize("m_attr_disown", ("m_uqmp_property", "m_uqcp_property")) -def test_uqp(m_attr_disown, msg): +@pytest.mark.parametrize("m_attr_readwrite", ("m_uqmp_readwrite", "m_uqcp_readwrite")) +def test_uqp(m_attr_readwrite, msg): outer = m.Outer() - assert getattr(outer, m_attr_disown) is None + assert getattr(outer, m_attr_readwrite) is None field_orig = m.Field() field_orig.num = 39 - setattr(outer, m_attr_disown, field_orig) + setattr(outer, m_attr_readwrite, field_orig) with pytest.raises(ValueError) as excinfo: field_orig.num assert ( msg(excinfo.value) == "Missing value for wrapped C++ type: Python instance was disowned." ) - field_retr1 = getattr(outer, m_attr_disown) - assert getattr(outer, m_attr_disown) is None + field_retr1 = getattr(outer, m_attr_readwrite) + assert getattr(outer, m_attr_readwrite) is None assert field_retr1.num == 39 field_retr1.num = 93 - setattr(outer, m_attr_disown, field_retr1) + setattr(outer, m_attr_readwrite, field_retr1) with pytest.raises(ValueError): field_retr1.num - field_retr2 = getattr(outer, m_attr_disown) + field_retr2 = getattr(outer, m_attr_readwrite) assert field_retr2.num == 93 @@ -108,13 +116,14 @@ def __delattr__(self, *args, **kwargs): return _dereference(self, delattr, *args, **kwargs) -@pytest.mark.parametrize("m_attr_disown", ("m_uqmp_property", "m_uqcp_property")) -def test_unique_ptr_field_proxy_poc(m_attr_disown, msg): +@pytest.mark.parametrize("m_attr", ("m_uqmp", "m_uqcp")) +def test_unique_ptr_field_proxy_poc(m_attr, msg): + m_attr_readwrite = m_attr + "_readwrite" outer = m.Outer() field_orig = m.Field() field_orig.num = 45 - setattr(outer, m_attr_disown, field_orig) - field_proxy = unique_ptr_field_proxy_poc(outer, m_attr_disown) + setattr(outer, m_attr_readwrite, field_orig) + field_proxy = unique_ptr_field_proxy_poc(outer, m_attr_readwrite) assert field_proxy.num == 45 assert field_proxy.num == 45 with pytest.raises(AttributeError): @@ -122,7 +131,7 @@ def test_unique_ptr_field_proxy_poc(m_attr_disown, msg): assert field_proxy.num == 45 field_proxy.num = 82 assert field_proxy.num == 82 - field_proxy = unique_ptr_field_proxy_poc(outer, m_attr_disown) + field_proxy = unique_ptr_field_proxy_poc(outer, m_attr_readwrite) assert field_proxy.num == 82 with pytest.raises(AttributeError): del field_proxy.num @@ -131,13 +140,19 @@ def test_unique_ptr_field_proxy_poc(m_attr_disown, msg): @pytest.mark.parametrize("m_attr", ("m_shmp", "m_shcp")) def test_shp(m_attr): + m_attr_readonly = m_attr + "_readonly" + m_attr_readwrite = m_attr + "_readwrite" outer = m.Outer() - assert getattr(outer, m_attr) is None + assert getattr(outer, m_attr_readonly) is None + assert getattr(outer, m_attr_readwrite) is None field = m.Field() field.num = 43 - setattr(outer, m_attr, field) - assert getattr(outer, m_attr).num == 43 - getattr(outer, m_attr).num = 57 + setattr(outer, m_attr_readwrite, field) + assert getattr(outer, m_attr_readonly).num == 43 + assert getattr(outer, m_attr_readwrite).num == 43 + getattr(outer, m_attr_readonly).num = 57 + getattr(outer, m_attr_readwrite).num = 57 assert field.num == 57 del field - assert getattr(outer, m_attr).num == 57 + assert getattr(outer, m_attr_readonly).num == 57 + assert getattr(outer, m_attr_readwrite).num == 57 From cca7dbb9a71754bcce00c62637c792d567fee2b0 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 20 Jan 2022 19:00:05 -0800 Subject: [PATCH 30/39] xetter_cpp_function implementation for std::unique_ptr --- include/pybind11/pybind11.h | 31 +++++++++++++++++++++++++++++++ tests/test_class_sh_property.cpp | 24 ++++-------------------- 2 files changed, 35 insertions(+), 20 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 5fd15cbb1f..0555fed934 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1448,6 +1448,7 @@ struct xetter_cpp_function< && !std::is_pointer::value // && !is_std_unique_ptr::value // && !is_std_shared_ptr::value>> { + template static cpp_function readonly(PM pm, const handle &hdl) { return cpp_function( @@ -1475,6 +1476,36 @@ struct xetter_cpp_function< } }; +template +struct xetter_cpp_function< + T, + D, + detail::enable_if_t< + detail::type_uses_smart_holder_type_caster::value // + && is_std_unique_ptr::value + && detail::type_uses_smart_holder_type_caster::value>> { + + template + static cpp_function readonly(PM, const handle &) { + static_assert(!is_std_unique_ptr::value, + "def_readonly cannot be used for std::unique_ptr members."); + return cpp_function{}; // Unreachable. + } + + template + static cpp_function read(PM pm, const handle &hdl) { + return cpp_function( + [pm](const std::shared_ptr &c_sp) -> D { return D{std::move(c_sp.get()->*pm)}; }, + is_method(hdl)); + } + + template + static cpp_function write(PM pm, const handle &hdl) { + return cpp_function([pm](T &c, D &&value) { (c.*pm).reset(value.release()); }, + is_method(hdl)); + } +}; + // clang-format off template diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index b5d815fc84..3ef5c8055e 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -73,26 +73,10 @@ TEST_SUBMODULE(class_sh_property, m) { .def_readonly("m_cptr_readonly", &Outer::m_cptr) .def_readwrite("m_cptr_readwrite", &Outer::m_cptr) - .def_property( // - "m_uqmp_readwrite", - [](const std::shared_ptr &self) { - return std::unique_ptr(std::move(self->m_uqmp)); - }, - [](Outer &self, std::unique_ptr uqmp) { - self.m_uqmp = std::move(uqmp); // - }) - .def_property( // - "m_uqcp_readwrite", - [](const std::shared_ptr &self) { - return std::unique_ptr(std::move(self->m_uqcp)); - }, - [](Outer &self, std::unique_ptr uqcp) { - self.m_uqcp = std::move(uqcp); // - }) - // .def_readonly("m_uqmp_readonly", &Outer::m_uqmp) // Compilation Error. - // .def_readwrite("m_uqmp_readwrite", &Outer::m_uqmp) - // .def_readonly("m_uqcp_readonly", &Outer::m_uqcp) // Compilation Error. - // .def_readwrite("m_uqcp_readwrite", &Outer::m_uqcp) + // .def_readonly("m_uqmp_readonly", &Outer::m_uqmp) // Custom compilation Error. + .def_readwrite("m_uqmp_readwrite", &Outer::m_uqmp) + // .def_readonly("m_uqcp_readonly", &Outer::m_uqcp) // Custom compilation Error. + .def_readwrite("m_uqcp_readwrite", &Outer::m_uqcp) .def_readwrite("m_shmp_readonly", &Outer::m_shmp) .def_readwrite("m_shmp_readwrite", &Outer::m_shmp) From 160acc8ec137b96a7653c6025c4f7b06645ecdf6 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 20 Jan 2022 19:50:46 -0800 Subject: [PATCH 31/39] Applying clang-tidy suggestion (looks much better). --- include/pybind11/pybind11.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 0555fed934..84c2f169cf 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1501,8 +1501,7 @@ struct xetter_cpp_function< template static cpp_function write(PM pm, const handle &hdl) { - return cpp_function([pm](T &c, D &&value) { (c.*pm).reset(value.release()); }, - is_method(hdl)); + return cpp_function([pm](T &c, D &&value) { c.*pm = std::move(value); }, is_method(hdl)); } }; From 50fe88d353d8e1047fd152573f60328abfd15156 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 21 Jan 2022 15:40:29 -0800 Subject: [PATCH 32/39] Polishing in pybind11.h: comments, moving helper functions out. --- .../detail/smart_holder_sfinae_hooks_only.h | 13 +++++ include/pybind11/pybind11.h | 49 +++++++++++-------- 2 files changed, 41 insertions(+), 21 deletions(-) diff --git a/include/pybind11/detail/smart_holder_sfinae_hooks_only.h b/include/pybind11/detail/smart_holder_sfinae_hooks_only.h index f324854751..cb43e94fba 100644 --- a/include/pybind11/detail/smart_holder_sfinae_hooks_only.h +++ b/include/pybind11/detail/smart_holder_sfinae_hooks_only.h @@ -6,6 +6,7 @@ #include "common.h" +#include #include #ifndef PYBIND11_USE_SMART_HOLDER_AS_DEFAULT @@ -29,5 +30,17 @@ struct smart_holder_type_caster_base_tag {}; template struct type_uses_smart_holder_type_caster; +// Simple helpers that may eventually be a better fit for common.h: + +template +struct is_std_unique_ptr : std::false_type {}; +template +struct is_std_unique_ptr> : std::true_type {}; + +template +struct is_std_shared_ptr : std::false_type {}; +template +struct is_std_shared_ptr> : std::true_type {}; + PYBIND11_NAMESPACE_END(detail) PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 84c2f169cf..38cf02eb43 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1378,8 +1378,11 @@ using default_holder_type = smart_holder; #endif -// TODO: move new code into detail namespace. - +// Classic (non-smart_holder) implementations for .def_readonly and .def_readwrite +// getter and setter functions. +// WARNING: This classic implementation can lead to dangling pointers for raw pointer members. +// See test_ptr() in tests/test_class_sh_property.py +// This implementation works as-is for smart_holder std::shared_ptr members. template struct xetter_cpp_function { template @@ -1398,16 +1401,12 @@ struct xetter_cpp_function { } }; -template -struct is_std_unique_ptr : std::false_type {}; -template -struct is_std_unique_ptr> : std::true_type {}; - -template -struct is_std_shared_ptr : std::false_type {}; -template -struct is_std_shared_ptr> : std::true_type {}; - +// smart_holder specializations for raw pointer members. +// WARNING: Like the classic implementation, this implementation can lead to dangling pointers. +// See test_ptr() in tests/test_class_sh_property.py +// However, the read functions return a shared_ptr to the member, emulating PyCLIF approach: +// https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233 +// This prevents disowning of the Python object owning the raw pointer member. template struct xetter_cpp_function< T, @@ -1439,15 +1438,19 @@ struct xetter_cpp_function< } }; +// smart_holder specializations for members held by-value. +// The read functions return a shared_ptr to the member, emulating PyCLIF approach: +// https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233 +// This prevents disowning of the Python object owning the member. template struct xetter_cpp_function< T, D, detail::enable_if_t::value && detail::type_uses_smart_holder_type_caster::value - && !std::is_pointer::value // - && !is_std_unique_ptr::value // - && !is_std_shared_ptr::value>> { + && !std::is_pointer::value // + && !detail::is_std_unique_ptr::value // + && !detail::is_std_shared_ptr::value>> { template static cpp_function readonly(PM pm, const handle &hdl) { @@ -1463,8 +1466,6 @@ struct xetter_cpp_function< static cpp_function read(PM pm, const handle &hdl) { return cpp_function( [pm](const std::shared_ptr &c_sp) -> std::shared_ptr { - // Emulating PyCLIF approach: - // https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233 return std::shared_ptr(c_sp, &(c_sp.get()->*pm)); }, is_method(hdl)); @@ -1476,18 +1477,24 @@ struct xetter_cpp_function< } }; +// smart_holder specializations for std::unique_ptr members. +// read disowns the member unique_ptr. +// write disowns the passed Python object. +// readonly is disabled (static_assert) because there is no safe & intuitive way to make the member +// accessible as a Python object without disowning the member unique_ptr. A .def_readonly disowning +// the unique_ptr member is deemed highly prone to misunderstandings. template struct xetter_cpp_function< T, D, detail::enable_if_t< detail::type_uses_smart_holder_type_caster::value // - && is_std_unique_ptr::value + && detail::is_std_unique_ptr::value && detail::type_uses_smart_holder_type_caster::value>> { template static cpp_function readonly(PM, const handle &) { - static_assert(!is_std_unique_ptr::value, + static_assert(!detail::is_std_unique_ptr::value, "def_readonly cannot be used for std::unique_ptr members."); return cpp_function{}; // Unreachable. } @@ -1727,8 +1734,8 @@ class class_ : public detail::generic_type { template class_ &def_readwrite_static(const char *name, D *pm, const Extra& ...extra) { - cpp_function fget([pm](const object &) -> const D & { return *pm; }, scope(*this)); - cpp_function fset([pm](const object &, const D &value) { *pm = value; }, scope(*this)); + cpp_function fget([pm](const object &) -> const D & { return *pm; }, scope(*this)), + fset([pm](const object &, const D &value) { *pm = value; }, scope(*this)); def_property_static(name, fget, fset, return_value_policy::reference, extra...); return *this; } From 4ad7135c11b881810d604c0d373a6ff30afc6528 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 21 Jan 2022 15:55:39 -0800 Subject: [PATCH 33/39] Polishing in test_class_sh_property.py: comments, moving code. --- tests/test_class_sh_property.py | 73 ++++++++++++++++++--------------- 1 file changed, 39 insertions(+), 34 deletions(-) diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index e055c44901..5200acc397 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -37,6 +37,26 @@ def test_valu_setter(): assert outer.m_valu_readwrite.num == 35 +@pytest.mark.parametrize("m_attr", ("m_shmp", "m_shcp")) +def test_shp(m_attr): + m_attr_readonly = m_attr + "_readonly" + m_attr_readwrite = m_attr + "_readwrite" + outer = m.Outer() + assert getattr(outer, m_attr_readonly) is None + assert getattr(outer, m_attr_readwrite) is None + field = m.Field() + field.num = 43 + setattr(outer, m_attr_readwrite, field) + assert getattr(outer, m_attr_readonly).num == 43 + assert getattr(outer, m_attr_readwrite).num == 43 + getattr(outer, m_attr_readonly).num = 57 + getattr(outer, m_attr_readwrite).num = 57 + assert field.num == 57 + del field + assert getattr(outer, m_attr_readonly).num == 57 + assert getattr(outer, m_attr_readwrite).num == 57 + + @pytest.mark.parametrize( "field_type, num_default, outer_type", [ @@ -90,30 +110,35 @@ def test_uqp(m_attr_readwrite, msg): assert field_retr2.num == 93 -def _dereference(proxy, xxxattr, *args, **kwargs): - obj = object.__getattribute__(proxy, "__obj") - field_name = object.__getattribute__(proxy, "__field_name") - field = getattr(obj, field_name) - assert field is not None - try: - return xxxattr(field, *args, **kwargs) - finally: - setattr(obj, field_name, field) - - +# Proof-of-concept (POC) for safe & intuitive Python access to unique_ptr members. +# The C++ member unique_ptr is disowned to a temporary Python object for accessing +# an attribute of the member. After the attribute was accessed, the Python object +# is disowned back to the C++ member unique_ptr. +# Productizing this POC is left for a future separate PR, as needed. class unique_ptr_field_proxy_poc(object): # noqa: N801 def __init__(self, obj, field_name): object.__setattr__(self, "__obj", obj) object.__setattr__(self, "__field_name", field_name) def __getattr__(self, *args, **kwargs): - return _dereference(self, getattr, *args, **kwargs) + return _proxy_dereference(self, getattr, *args, **kwargs) def __setattr__(self, *args, **kwargs): - return _dereference(self, setattr, *args, **kwargs) + return _proxy_dereference(self, setattr, *args, **kwargs) def __delattr__(self, *args, **kwargs): - return _dereference(self, delattr, *args, **kwargs) + return _proxy_dereference(self, delattr, *args, **kwargs) + + +def _proxy_dereference(proxy, xxxattr, *args, **kwargs): + obj = object.__getattribute__(proxy, "__obj") + field_name = object.__getattribute__(proxy, "__field_name") + field = getattr(obj, field_name) # Disowns the C++ unique_ptr member. + assert field is not None + try: + return xxxattr(field, *args, **kwargs) + finally: + setattr(obj, field_name, field) # Disowns the temporary Python object (field). @pytest.mark.parametrize("m_attr", ("m_uqmp", "m_uqcp")) @@ -136,23 +161,3 @@ def test_unique_ptr_field_proxy_poc(m_attr, msg): with pytest.raises(AttributeError): del field_proxy.num assert field_proxy.num == 82 - - -@pytest.mark.parametrize("m_attr", ("m_shmp", "m_shcp")) -def test_shp(m_attr): - m_attr_readonly = m_attr + "_readonly" - m_attr_readwrite = m_attr + "_readwrite" - outer = m.Outer() - assert getattr(outer, m_attr_readonly) is None - assert getattr(outer, m_attr_readwrite) is None - field = m.Field() - field.num = 43 - setattr(outer, m_attr_readwrite, field) - assert getattr(outer, m_attr_readonly).num == 43 - assert getattr(outer, m_attr_readwrite).num == 43 - getattr(outer, m_attr_readonly).num = 57 - getattr(outer, m_attr_readwrite).num = 57 - assert field.num == 57 - del field - assert getattr(outer, m_attr_readonly).num == 57 - assert getattr(outer, m_attr_readwrite).num == 57 From ef81de96a5ac36c9dbfed8dded33ffac1efb5d75 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 21 Jan 2022 15:58:35 -0800 Subject: [PATCH 34/39] Minor tweak to comment. --- include/pybind11/detail/smart_holder_sfinae_hooks_only.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/detail/smart_holder_sfinae_hooks_only.h b/include/pybind11/detail/smart_holder_sfinae_hooks_only.h index cb43e94fba..ca57cb61cb 100644 --- a/include/pybind11/detail/smart_holder_sfinae_hooks_only.h +++ b/include/pybind11/detail/smart_holder_sfinae_hooks_only.h @@ -30,7 +30,7 @@ struct smart_holder_type_caster_base_tag {}; template struct type_uses_smart_holder_type_caster; -// Simple helpers that may eventually be a better fit for common.h: +// Simple helpers that may eventually be a better fit for another header file: template struct is_std_unique_ptr : std::false_type {}; From b0ef8ac44f9c2a2791f822f515705c471393ace8 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 27 Jan 2022 16:51:02 -0800 Subject: [PATCH 35/39] Adjustment after `git rebase -X theirs smart_holder` --- tests/CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index a23cb2d44a..e304b88f0a 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -137,6 +137,7 @@ set(PYBIND11_TEST_FILES test_class_sh_trampoline_unique_ptr test_class_sh_unique_ptr_member test_class_sh_virtual_py_cpp_mix + test_class_sh_void_ptr_capsule test_classh_mock test_const_name test_constants_and_functions From 2d1f958de65608cda3ce5a0bb5d7c4a7f00d8a5a Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 27 Jan 2022 19:31:32 -0800 Subject: [PATCH 36/39] must_be_member_function_pointer --- include/pybind11/pybind11.h | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 38cf02eb43..bdabf2673d 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1378,6 +1378,10 @@ using default_holder_type = smart_holder; #endif +template +using must_be_member_function_pointer + = detail::enable_if_t::value, int>; + // Classic (non-smart_holder) implementations for .def_readonly and .def_readwrite // getter and setter functions. // WARNING: This classic implementation can lead to dangling pointers for raw pointer members. @@ -1385,17 +1389,17 @@ using default_holder_type = smart_holder; // This implementation works as-is for smart_holder std::shared_ptr members. template struct xetter_cpp_function { - template + template = 0> static cpp_function readonly(PM pm, const handle &hdl) { return cpp_function([pm](const T &c) -> const D & { return c.*pm; }, is_method(hdl)); } - template + template = 0> static cpp_function read(PM pm, const handle &hdl) { return readonly(pm, hdl); } - template + template = 0> static cpp_function write(PM pm, const handle &hdl) { return cpp_function([pm](T &c, const D &value) { c.*pm = value; }, is_method(hdl)); } @@ -1417,7 +1421,7 @@ struct xetter_cpp_function< using drp = typename std::remove_pointer::type; - template + template = 0> static cpp_function readonly(PM pm, const handle &hdl) { return cpp_function( [pm](const std::shared_ptr &c_sp) -> std::shared_ptr { @@ -1427,12 +1431,12 @@ struct xetter_cpp_function< is_method(hdl)); } - template + template = 0> static cpp_function read(PM pm, const handle &hdl) { return readonly(pm, hdl); } - template + template = 0> static cpp_function write(PM pm, const handle &hdl) { return cpp_function([pm](T &c, D value) { c.*pm = value; }, is_method(hdl)); } @@ -1452,7 +1456,7 @@ struct xetter_cpp_function< && !detail::is_std_unique_ptr::value // && !detail::is_std_shared_ptr::value>> { - template + template = 0> static cpp_function readonly(PM pm, const handle &hdl) { return cpp_function( [pm](const std::shared_ptr &c_sp) @@ -1462,7 +1466,7 @@ struct xetter_cpp_function< is_method(hdl)); } - template + template = 0> static cpp_function read(PM pm, const handle &hdl) { return cpp_function( [pm](const std::shared_ptr &c_sp) -> std::shared_ptr { @@ -1471,7 +1475,7 @@ struct xetter_cpp_function< is_method(hdl)); } - template + template = 0> static cpp_function write(PM pm, const handle &hdl) { return cpp_function([pm](T &c, const D &value) { c.*pm = value; }, is_method(hdl)); } @@ -1492,21 +1496,21 @@ struct xetter_cpp_function< && detail::is_std_unique_ptr::value && detail::type_uses_smart_holder_type_caster::value>> { - template + template = 0> static cpp_function readonly(PM, const handle &) { static_assert(!detail::is_std_unique_ptr::value, "def_readonly cannot be used for std::unique_ptr members."); return cpp_function{}; // Unreachable. } - template + template = 0> static cpp_function read(PM pm, const handle &hdl) { return cpp_function( [pm](const std::shared_ptr &c_sp) -> D { return D{std::move(c_sp.get()->*pm)}; }, is_method(hdl)); } - template + template = 0> static cpp_function write(PM pm, const handle &hdl) { return cpp_function([pm](T &c, D &&value) { c.*pm = std::move(value); }, is_method(hdl)); } From 5737c439f8b76749fe023888fcfe56e6492d243b Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 27 Jan 2022 20:02:56 -0800 Subject: [PATCH 37/39] Making use of all_of, none_of as suggested by @Skylion007 --- include/pybind11/pybind11.h | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index bdabf2673d..f72a5ea63a 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1415,9 +1415,9 @@ template struct xetter_cpp_function< T, D, - detail::enable_if_t::value - && detail::type_uses_smart_holder_type_caster::value - && std::is_pointer::value>> { + detail::enable_if_t, + detail::type_uses_smart_holder_type_caster, + std::is_pointer>::value>> { using drp = typename std::remove_pointer::type; @@ -1450,11 +1450,11 @@ template struct xetter_cpp_function< T, D, - detail::enable_if_t::value - && detail::type_uses_smart_holder_type_caster::value - && !std::is_pointer::value // - && !detail::is_std_unique_ptr::value // - && !detail::is_std_shared_ptr::value>> { + detail::enable_if_t, + detail::type_uses_smart_holder_type_caster, + detail::none_of, + detail::is_std_unique_ptr, + detail::is_std_shared_ptr>>::value>> { template = 0> static cpp_function readonly(PM pm, const handle &hdl) { @@ -1491,10 +1491,10 @@ template struct xetter_cpp_function< T, D, - detail::enable_if_t< - detail::type_uses_smart_holder_type_caster::value // - && detail::is_std_unique_ptr::value - && detail::type_uses_smart_holder_type_caster::value>> { + detail::enable_if_t, + detail::is_std_unique_ptr, + detail::type_uses_smart_holder_type_caster>::value>> { template = 0> static cpp_function readonly(PM, const handle &) { From 1eb3bc3fb74a1c4d9f44c3cd7b1b1f8b90661fb5 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 28 Jan 2022 01:00:03 -0800 Subject: [PATCH 38/39] Adding comments (no code changes). --- include/pybind11/pybind11.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index f72a5ea63a..1809d0c21f 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1378,10 +1378,20 @@ using default_holder_type = smart_holder; #endif +// Helper for the xetter_cpp_function static member functions below. xetter_cpp_function is +// a shortcut for 'getter & setter adapters for .def_readonly & .def_readwrite'. +// The only purpose of these adapters is to support .def_readonly & .def_readwrite. +// In this context, the PM template parameter is certain to be a Pointer to a Member. +// The main purpose of must_be_member_function_pointer is to make this obvious, and to guard +// against accidents. As a side-effect, it also explains why the syntactical clutter for +// perfect forwarding is not needed. template using must_be_member_function_pointer = detail::enable_if_t::value, int>; +// Note that xetter_cpp_function is intentionally in the main pybind11 namespace, +// because user-defined specializations could be useful. + // Classic (non-smart_holder) implementations for .def_readonly and .def_readwrite // getter and setter functions. // WARNING: This classic implementation can lead to dangling pointers for raw pointer members. From 64c5c4b24e038a57606d07c5f405f01f7fd4da09 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 1 Feb 2022 16:08:00 -0800 Subject: [PATCH 39/39] Adding comment: compact 4-character naming --- tests/test_class_sh_property.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index 3ef5c8055e..a5aaebe1fa 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -20,6 +20,8 @@ struct Field { }; struct Outer { + // The compact 4-character naming matches that in test_class_sh_basic.cpp + // (c = const, m = mutable). Field m_valu; Field *m_mptr = nullptr; const Field *m_cptr = nullptr;