From fa0ad8aa6df27e2e02c4420b0ec8592046bb1b62 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 2 Apr 2022 01:22:04 -0700 Subject: [PATCH 1/7] Transferred net diff from PR #3581, as-is. --- .../detail/smart_holder_sfinae_hooks_only.h | 13 ++ include/pybind11/pybind11.h | 164 +++++++++++++++++- tests/CMakeLists.txt | 1 + tests/test_class_sh_property.cpp | 90 ++++++++++ tests/test_class_sh_property.py | 163 +++++++++++++++++ 5 files changed, 426 insertions(+), 5 deletions(-) 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_sfinae_hooks_only.h b/include/pybind11/detail/smart_holder_sfinae_hooks_only.h index f324854751..ca57cb61cb 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 another header file: + +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 9f00800430..8a09e5be3f 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1514,6 +1514,154 @@ 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. +// 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 = 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 = 0> + static cpp_function read(PM pm, const handle &hdl) { + return readonly(pm, hdl); + } + + 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)); + } +}; + +// 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, + D, + detail::enable_if_t, + detail::type_uses_smart_holder_type_caster, + std::is_pointer>::value>> { + + using drp = typename std::remove_pointer::type; + + template = 0> + 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; + return std::shared_ptr(c_sp, ptr); + }, + is_method(hdl)); + } + + template = 0> + static cpp_function read(PM pm, const handle &hdl) { + return readonly(pm, hdl); + } + + 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)); + } +}; + +// 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, + 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) { + return cpp_function( + [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 = 0> + static cpp_function read(PM pm, const handle &hdl) { + return cpp_function( + [pm](const std::shared_ptr &c_sp) -> std::shared_ptr { + return std::shared_ptr(c_sp, &(c_sp.get()->*pm)); + }, + is_method(hdl)); + } + + 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)); + } +}; + +// 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::is_std_unique_ptr, + detail::type_uses_smart_holder_type_caster>::value>> { + + 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 = 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 = 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)); + } +}; + template class class_ : public detail::generic_type { template @@ -1727,9 +1875,12 @@ class class_ : public detail::generic_type { 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, + xetter_cpp_function::read(pm, *this), + xetter_cpp_function::write(pm, *this), + return_value_policy::reference_internal, + extra...); return *this; } @@ -1737,8 +1888,11 @@ class class_ : public detail::generic_type { 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, + xetter_cpp_function::readonly(pm, *this), + return_value_policy::reference_internal, + extra...); return *this; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d9086f39b1..dd351d69a9 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -128,6 +128,7 @@ set(PYBIND11_TEST_FILES 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 diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp new file mode 100644 index 0000000000..a5aaebe1fa --- /dev/null +++ b/tests/test_class_sh_property.cpp @@ -0,0 +1,90 @@ +#include "pybind11_tests.h" + +#include "pybind11/smart_holder.h" + +#include + +namespace test_class_sh_property { + +struct ClassicField { + int num = -88; +}; + +struct ClassicOuter { + ClassicField *m_mptr = nullptr; + const ClassicField *m_cptr = nullptr; +}; + +struct Field { + int num = -99; +}; + +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; + 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) + +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_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") // + .def(py::init<>()) // + .def_readwrite("num", &Field::num) // + ; + + py::classh(m, "Outer") // + .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_readwrite", &Outer::m_mptr) + .def_readonly("m_cptr_readonly", &Outer::m_cptr) + .def_readwrite("m_cptr_readwrite", &Outer::m_cptr) + + // .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) + .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 new file mode 100644 index 0000000000..5200acc397 --- /dev/null +++ b/tests/test_class_sh_property.py @@ -0,0 +1,163 @@ +# -*- 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 field` is apparently deferred") +@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 = getattr(outer, m_attr) + 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 field + m.DisownOuter(outer) + with pytest.raises(ValueError) as excinfo: + getattr(outer, m_attr) + assert ( + msg(excinfo.value) + == "Missing value for wrapped C++ type: Python instance was disowned." + ) + + +def test_valu_setter(): + outer = m.Outer() + assert outer.m_valu_readonly.num == -99 + assert outer.m_valu_readwrite.num == -99 + field = m.Field() + field.num = 35 + outer.m_valu_readwrite = field + assert outer.m_valu_readonly.num == 35 + 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", + [ + (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, m_attr): + m_attr_readonly = m_attr + "_readonly" + m_attr_readwrite = m_attr + "_readwrite" + outer = outer_type() + 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_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_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_readonly).num == 76 + assert getattr(outer, m_attr_readwrite).num == 76 + + +@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_readwrite) is None + field_orig = m.Field() + field_orig.num = 39 + 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_readwrite) + assert getattr(outer, m_attr_readwrite) is None + assert field_retr1.num == 39 + field_retr1.num = 93 + setattr(outer, m_attr_readwrite, field_retr1) + with pytest.raises(ValueError): + field_retr1.num + field_retr2 = getattr(outer, m_attr_readwrite) + assert field_retr2.num == 93 + + +# 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 _proxy_dereference(self, getattr, *args, **kwargs) + + def __setattr__(self, *args, **kwargs): + return _proxy_dereference(self, setattr, *args, **kwargs) + + def __delattr__(self, *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")) +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_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): + 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_readwrite) + assert field_proxy.num == 82 + with pytest.raises(AttributeError): + del field_proxy.num + assert field_proxy.num == 82 From 68305a3abf12ca5acf2d28f94f5441f100d94e72 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 2 Apr 2022 01:30:08 -0700 Subject: [PATCH 2/7] Automatic `pre-commit run --all-files` fixes. NO manual changes. --- include/pybind11/pybind11.h | 20 +++++++++----------- tests/test_class_sh_property.cpp | 3 +-- tests/test_class_sh_property.py | 3 +-- 3 files changed, 11 insertions(+), 15 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 8a09e5be3f..791e1414ce 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1875,12 +1875,11 @@ class class_ : public detail::generic_type { 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)"); - def_property( - name, - xetter_cpp_function::read(pm, *this), - xetter_cpp_function::write(pm, *this), - return_value_policy::reference_internal, - extra...); + def_property(name, + xetter_cpp_function::read(pm, *this), + xetter_cpp_function::write(pm, *this), + return_value_policy::reference_internal, + extra...); return *this; } @@ -1888,11 +1887,10 @@ class class_ : public detail::generic_type { 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)"); - def_property_readonly( - name, - xetter_cpp_function::readonly(pm, *this), - return_value_policy::reference_internal, - extra...); + def_property_readonly(name, + xetter_cpp_function::readonly(pm, *this), + return_value_policy::reference_internal, + extra...); return *this; } diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index a5aaebe1fa..53708ecc8e 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -1,6 +1,5 @@ -#include "pybind11_tests.h" - #include "pybind11/smart_holder.h" +#include "pybind11_tests.h" #include diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index 5200acc397..9891d0d493 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -1,4 +1,3 @@ -# -*- coding: utf-8 -*- import pytest import env # noqa: F401 @@ -115,7 +114,7 @@ def test_uqp(m_attr_readwrite, msg): # 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 +class unique_ptr_field_proxy_poc: # noqa: N801 def __init__(self, obj, field_name): object.__setattr__(self, "__obj", obj) object.__setattr__(self, "__field_name", field_name) From 7dee4ad8dff4513bf2e7d198f8ab123e209b66d9 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 2 Apr 2022 01:34:53 -0700 Subject: [PATCH 3/7] Removing trailing `//` (originally added to manipulate clang-format), as suggested by @charlesbeattie back in Jan/Feb under PR #3581. --- tests/test_class_sh_property.cpp | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index 53708ecc8e..2d90fdb3cc 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -46,24 +46,19 @@ TEST_SUBMODULE(class_sh_property, m) { using namespace test_class_sh_property; py::class_>(m, "ClassicField") - .def(py::init<>()) // - .def_readwrite("num", &ClassicField::num) // - ; + .def(py::init<>()) + .def_readwrite("num", &ClassicField::num); py::class_>(m, "ClassicOuter") .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) // - ; + .def_readwrite("m_cptr_readwrite", &ClassicOuter::m_cptr); - py::classh(m, "Field") // - .def(py::init<>()) // - .def_readwrite("num", &Field::num) // - ; + py::classh(m, "Field").def(py::init<>()).def_readwrite("num", &Field::num); - py::classh(m, "Outer") // + py::classh(m, "Outer") .def(py::init<>()) .def_readonly("m_valu_readonly", &Outer::m_valu) @@ -82,8 +77,7 @@ TEST_SUBMODULE(class_sh_property, m) { .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) // - ; + .def_readwrite("m_shcp_readwrite", &Outer::m_shcp); m.def("DisownOuter", DisownOuter); } From 07c807c5ba4f6006804dedf6525edbfef2eb53f3 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 13 May 2022 07:32:59 -0700 Subject: [PATCH 4/7] Renaming `xetter_cpp_function` to `property_cpp_function` as suggested by @rainwoodman --- include/pybind11/pybind11.h | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 791e1414ce..4330dcd3ad 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1514,27 +1514,26 @@ 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. +// Helper for the property_cpp_function static member functions below. +// The only purpose of these functions 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 +// against accidents. As a side-effect, it also explains why the syntactical overhead 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, +// Note that property_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. // See test_ptr() in tests/test_class_sh_property.py -// This implementation works as-is for smart_holder std::shared_ptr members. +// This implementation works as-is (and safely) for smart_holder std::shared_ptr members. template -struct xetter_cpp_function { +struct property_cpp_function { 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)); @@ -1554,11 +1553,11 @@ struct xetter_cpp_function { // 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: +// However, the read functions return a shared_ptr to the member, emulating the 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< +struct property_cpp_function< T, D, detail::enable_if_t, @@ -1589,11 +1588,11 @@ 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: +// The read functions return a shared_ptr to the member, emulating the 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< +struct property_cpp_function< T, D, detail::enable_if_t, @@ -1634,7 +1633,7 @@ struct xetter_cpp_function< // 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< +struct property_cpp_function< T, D, detail::enable_if_t::value || std::is_base_of::value, "def_readwrite() requires a class member (or base class member)"); def_property(name, - xetter_cpp_function::read(pm, *this), - xetter_cpp_function::write(pm, *this), + property_cpp_function::read(pm, *this), + property_cpp_function::write(pm, *this), return_value_policy::reference_internal, extra...); return *this; @@ -1888,7 +1887,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::readonly(pm, *this), + property_cpp_function::readonly(pm, *this), return_value_policy::reference_internal, extra...); return *this; From fc777472ddf984bc9c6fa599c018758fb0882d09 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 13 May 2022 07:46:03 -0700 Subject: [PATCH 5/7] Fully explain the terse variable naming scheme in test_class_sh_property (as suggested by @rainwoodman) --- .codespell-ignorelines | 1 + tests/test_class_sh_property.cpp | 7 +++++-- tests/test_class_sh_property.py | 3 +++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/.codespell-ignorelines b/.codespell-ignorelines index 5781de01a9..01ff575b40 100644 --- a/.codespell-ignorelines +++ b/.codespell-ignorelines @@ -11,3 +11,4 @@ atyp_valu rtrn_valu() { atyp_valu obj{"Valu"}; return obj; } valu = other.valu; with pytest.raises(ValueError) as excinfo: with pytest.raises(ValueError) as exc_info: +// valu(e), ref(erence), ptr or p (pointer), r = rvalue, m = mutable, c = const, diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp index 2d90fdb3cc..35615cec66 100644 --- a/tests/test_class_sh_property.cpp +++ b/tests/test_class_sh_property.cpp @@ -1,3 +1,8 @@ +// The compact 4-character naming matches that in test_class_sh_basic.cpp +// Variable names are intentionally terse, to not distract from the more important C++ type names: +// valu(e), ref(erence), ptr or p (pointer), r = rvalue, m = mutable, c = const, +// sh = shared_ptr, uq = unique_ptr. + #include "pybind11/smart_holder.h" #include "pybind11_tests.h" @@ -19,8 +24,6 @@ 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; diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index 9891d0d493..84b8674729 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -1,3 +1,6 @@ +# The compact 4-character naming scheme (e.g. mptr, cptr, shcp) is explained at the top of +# test_class_sh_property.cpp. + import pytest import env # noqa: F401 From 8ec57216b4cec0ff19a3e7333aad91b150d9cbdc Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 13 May 2022 08:07:27 -0700 Subject: [PATCH 6/7] Also use parametrize for readonly, readwrite (as suggested by @rainwoodman) --- tests/test_class_sh_property.py | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index 84b8674729..19dd976feb 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -67,25 +67,21 @@ def test_shp(m_attr): ], ) @pytest.mark.parametrize("m_attr", ("m_mptr", "m_cptr")) -def test_ptr(field_type, num_default, outer_type, m_attr): - m_attr_readonly = m_attr + "_readonly" - m_attr_readwrite = m_attr + "_readwrite" +@pytest.mark.parametrize("r_kind", ("_readonly", "_readwrite")) +def test_ptr(field_type, num_default, outer_type, m_attr, r_kind): + m_attr_r_kind = m_attr + r_kind outer = outer_type() - assert getattr(outer, m_attr_readonly) is None - assert getattr(outer, m_attr_readwrite) is None + assert getattr(outer, m_attr_r_kind) is None field = field_type() assert field.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 + setattr(outer, m_attr + "_readwrite", field) + assert getattr(outer, m_attr_r_kind).num == num_default field.num = 76 - assert getattr(outer, m_attr_readonly).num == 76 - assert getattr(outer, m_attr_readwrite).num == 76 + assert getattr(outer, m_attr_r_kind).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_readonly).num == 76 - assert getattr(outer, m_attr_readwrite).num == 76 + assert getattr(outer, m_attr_r_kind).num == 76 @pytest.mark.parametrize("m_attr_readwrite", ("m_uqmp_readwrite", "m_uqcp_readwrite")) From 3c4168c3d363f3e94d73bf6e4cb9abe540507c3c Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 17 May 2022 16:04:06 -0700 Subject: [PATCH 7/7] Apply change suggested by @skylion007 (with clang-format). --- include/pybind11/pybind11.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 4330dcd3ad..989ec49fed 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1583,7 +1583,8 @@ struct property_cpp_function< 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)); + return cpp_function([pm](T &c, D value) { c.*pm = std::forward(value); }, + is_method(hdl)); } };