From 03f627ebb1884e3f532d96707454cc3051eb98bd Mon Sep 17 00:00:00 2001 From: Dean Moldovan Date: Tue, 1 Nov 2016 11:44:57 +0100 Subject: [PATCH] Make reference(_internal) the default return value policy for properties (#473) * Make reference(_internal) the default return value policy for properties Before this, all `def_property*` functions used `automatic` as their default return value policy. This commit makes it so that: * Non-static properties use `reference_interal` by default, thus matching `def_readonly` and `def_readwrite`. * Static properties use `reference` by default, thus matching `def_readonly_static` and `def_readwrite_static`. In case `cpp_function` is passed to any `def_property*`, its policy will be used instead of any defaults. User-defined arguments in `extras` still have top priority and will override both the default policies and the ones from `cpp_function`. Resolves #436. * Almost always use return_value_policy::move for rvalues For functions which return rvalues or rvalue references, the only viable return value policies are `copy` and `move`. `reference(_internal)` and `take_ownership` would take the address of a temporary which is always an error. This commit prevents possible user errors by overriding the bad rvalue policies with `move`. Besides `move`, only `copy` is allowed, and only if it's explicitly selected by the user. This is also a necessary safety feature to support the new default return value policies for properties: `reference(_internal)`. --- docs/advanced/functions.rst | 16 ++++++++-- docs/changelog.rst | 2 ++ include/pybind11/cast.h | 2 +- include/pybind11/pybind11.h | 34 ++++++++++++++++++--- tests/test_methods_and_attributes.cpp | 44 +++++++++++++++++++++++++++ tests/test_methods_and_attributes.py | 38 +++++++++++++++++++++++ 6 files changed, 128 insertions(+), 8 deletions(-) diff --git a/docs/advanced/functions.rst b/docs/advanced/functions.rst index d9886e2a2..f291e8222 100644 --- a/docs/advanced/functions.rst +++ b/docs/advanced/functions.rst @@ -100,13 +100,23 @@ The following table provides an overview of available policies: | | (i.e. via handle::operator()). You probably won't need to use this. | +--------------------------------------------------+----------------------------------------------------------------------------+ -Return value policies can also be applied to properties, in which case the -arguments must be passed through the :class:`cpp_function` constructor: +Return value policies can also be applied to properties: .. code-block:: cpp class_(m, "MyClass") - def_property("data" + .def_property("data", &MyClass::getData, &MyClass::setData, + py::return_value_policy::copy); + +Technically, the code above applies the policy to both the getter and the +setter function, however, the setter doesn't really care about *return* +value policies which makes this a convenient terse syntax. Alternatively, +targeted arguments can be passed through the :class:`cpp_function` constructor: + +.. code-block:: cpp + + class_(m, "MyClass") + .def_property("data" py::cpp_function(&MyClass::getData, py::return_value_policy::copy), py::cpp_function(&MyClass::setData) ); diff --git a/docs/changelog.rst b/docs/changelog.rst index 0c3ec1bcb..e837aa426 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -57,6 +57,8 @@ Breaking changes queued for v2.0.0 (Not yet released) to chain attributes ``obj.attr("a")[key].attr("b").attr("method")(1, 2, 3)```. * Added built-in support for ``std::shared_ptr`` holder type. There is no more need to do it manually via ``PYBIND11_DECLARE_HOLDER_TYPE(T, std::shared_ptr)``. +* Default return values policy changes: non-static properties now use ``reference_internal`` + and static properties use ``reference`` (previous default was ``automatic``, i.e. ``copy``). * Various minor improvements of library internals (no user-visible changes) 1.8.1 (July 12, 2016) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index a910b00c4..dd81d9a04 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -345,7 +345,7 @@ public: } static handle cast(itype &&src, return_value_policy policy, handle parent) { - if (policy == return_value_policy::automatic || policy == return_value_policy::automatic_reference) + if (policy != return_value_policy::copy) policy = return_value_policy::move; return cast(&src, policy, parent); } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 989219611..10c0e7a28 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1054,23 +1054,49 @@ public: return *this; } + /// Uses return_value_policy::reference_internal by default + template + class_ &def_property_readonly(const char *name, const Getter &fget, const Extra& ...extra) { + return def_property_readonly(name, cpp_function(fget), return_value_policy::reference_internal, extra...); + } + + /// Uses cpp_function's return_value_policy by default template class_ &def_property_readonly(const char *name, const cpp_function &fget, const Extra& ...extra) { - def_property(name, fget, cpp_function(), extra...); - return *this; + return def_property(name, fget, cpp_function(), extra...); } + /// Uses return_value_policy::reference by default + template + class_ &def_property_readonly_static(const char *name, const Getter &fget, const Extra& ...extra) { + return def_property_readonly_static(name, cpp_function(fget), return_value_policy::reference, extra...); + } + + /// Uses cpp_function's return_value_policy by default template class_ &def_property_readonly_static(const char *name, const cpp_function &fget, const Extra& ...extra) { - def_property_static(name, fget, cpp_function(), extra...); - return *this; + return def_property_static(name, fget, cpp_function(), extra...); } + /// Uses return_value_policy::reference_internal by default + template + class_ &def_property(const char *name, const Getter &fget, const cpp_function &fset, const Extra& ...extra) { + return def_property(name, cpp_function(fget), fset, return_value_policy::reference_internal, extra...); + } + + /// Uses cpp_function's return_value_policy by default template class_ &def_property(const char *name, const cpp_function &fget, const cpp_function &fset, const Extra& ...extra) { return def_property_static(name, fget, fset, is_method(*this), extra...); } + /// Uses return_value_policy::reference by default + template + class_ &def_property_static(const char *name, const Getter &fget, const cpp_function &fset, const Extra& ...extra) { + return def_property_static(name, cpp_function(fget), fset, return_value_policy::reference, extra...); + } + + /// Uses cpp_function's return_value_policy by default template class_ &def_property_static(const char *name, const cpp_function &fget, const cpp_function &fset, const Extra& ...extra) { auto rec_fget = get_function_record(fget), rec_fset = get_function_record(fset); diff --git a/tests/test_methods_and_attributes.cpp b/tests/test_methods_and_attributes.cpp index 82c81f724..ab59d8c21 100644 --- a/tests/test_methods_and_attributes.cpp +++ b/tests/test_methods_and_attributes.cpp @@ -66,6 +66,24 @@ struct TestProperties { int TestProperties::static_value = 1; +struct SimpleValue { int value = 1; }; + +struct TestPropRVP { + SimpleValue v1; + SimpleValue v2; + static SimpleValue sv1; + static SimpleValue sv2; + + const SimpleValue &get1() const { return v1; } + const SimpleValue &get2() const { return v2; } + SimpleValue get_rvalue() const { return v2; } + void set1(int v) { v1.value = v; } + void set2(int v) { v2.value = v; } +}; + +SimpleValue TestPropRVP::sv1{}; +SimpleValue TestPropRVP::sv2{}; + class DynamicClass { public: DynamicClass() { print_default_created(this); } @@ -117,6 +135,32 @@ test_initializer methods_and_attributes([](py::module &m) { [](py::object) { return TestProperties::static_get(); }, [](py::object, int v) { return TestProperties::static_set(v); }); + py::class_(m, "SimpleValue") + .def_readwrite("value", &SimpleValue::value); + + auto static_get1 = [](py::object) -> const SimpleValue & { return TestPropRVP::sv1; }; + auto static_get2 = [](py::object) -> const SimpleValue & { return TestPropRVP::sv2; }; + auto static_set1 = [](py::object, int v) { TestPropRVP::sv1.value = v; }; + auto static_set2 = [](py::object, int v) { TestPropRVP::sv2.value = v; }; + auto rvp_copy = py::return_value_policy::copy; + + py::class_(m, "TestPropRVP") + .def(py::init<>()) + .def_property_readonly("ro_ref", &TestPropRVP::get1) + .def_property_readonly("ro_copy", &TestPropRVP::get2, rvp_copy) + .def_property_readonly("ro_func", py::cpp_function(&TestPropRVP::get2, rvp_copy)) + .def_property("rw_ref", &TestPropRVP::get1, &TestPropRVP::set1) + .def_property("rw_copy", &TestPropRVP::get2, &TestPropRVP::set2, rvp_copy) + .def_property("rw_func", py::cpp_function(&TestPropRVP::get2, rvp_copy), &TestPropRVP::set2) + .def_property_readonly_static("static_ro_ref", static_get1) + .def_property_readonly_static("static_ro_copy", static_get2, rvp_copy) + .def_property_readonly_static("static_ro_func", py::cpp_function(static_get2, rvp_copy)) + .def_property_static("static_rw_ref", static_get1, static_set1) + .def_property_static("static_rw_copy", static_get2, static_set2, rvp_copy) + .def_property_static("static_rw_func", py::cpp_function(static_get2, rvp_copy), static_set2) + .def_property_readonly("rvalue", &TestPropRVP::get_rvalue) + .def_property_readonly_static("static_rvalue", [](py::object) { return SimpleValue(); }); + py::class_(m, "DynamicClass", py::dynamic_attr()) .def(py::init()); diff --git a/tests/test_methods_and_attributes.py b/tests/test_methods_and_attributes.py index f4116c3b6..f97dd4e7e 100644 --- a/tests/test_methods_and_attributes.py +++ b/tests/test_methods_and_attributes.py @@ -85,6 +85,44 @@ def test_static_properties(): assert Type.def_property_static == 3 +@pytest.mark.parametrize("access", ["ro", "rw", "static_ro", "static_rw"]) +def test_property_return_value_policies(access): + from pybind11_tests import TestPropRVP + + if not access.startswith("static"): + obj = TestPropRVP() + else: + obj = TestPropRVP + + ref = getattr(obj, access + "_ref") + assert ref.value == 1 + ref.value = 2 + assert getattr(obj, access + "_ref").value == 2 + ref.value = 1 # restore original value for static properties + + copy = getattr(obj, access + "_copy") + assert copy.value == 1 + copy.value = 2 + assert getattr(obj, access + "_copy").value == 1 + + copy = getattr(obj, access + "_func") + assert copy.value == 1 + copy.value = 2 + assert getattr(obj, access + "_func").value == 1 + + +def test_property_rvalue_policy(): + """When returning an rvalue, the return value policy is automatically changed from + `reference(_internal)` to `move`. The following would not work otherwise.""" + from pybind11_tests import TestPropRVP + + instance = TestPropRVP() + o = instance.rvalue + assert o.value == 1 + o = TestPropRVP.static_rvalue + assert o.value == 1 + + def test_dynamic_attributes(): from pybind11_tests import DynamicClass, CppDerivedDynamicClass