From d079f41c26296d1b1fd054a20bf6a298adcc0607 Mon Sep 17 00:00:00 2001 From: Dean Moldovan Date: Sun, 20 Nov 2016 05:31:02 +0100 Subject: [PATCH] Always use return_value_policy::move for rvalues (#510) Fixes #509. The move policy was already set for rvalues in PR #473, but this only applied to directly cast user-defined types. The problem is that STL containers cast values indirectly and the rvalue information is lost. Therefore the move policy was not set correctly. This commit fixes it. This also makes an additional adjustment to remove the `copy` policy exception: rvalues now always use the `move` policy. This is also safe for copy-only rvalues because the `move` policy has an internal fallback to copying. --- include/pybind11/cast.h | 6 ++---- include/pybind11/pybind11.h | 7 ++++++- tests/test_python_types.cpp | 14 ++++++++++++++ tests/test_python_types.py | 12 ++++++++++++ tools/libsize.py | 2 +- 5 files changed, 35 insertions(+), 6 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index d6210d3b1..a4e47d38f 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -380,10 +380,8 @@ public: return cast(&src, policy, parent); } - static handle cast(itype &&src, return_value_policy policy, handle parent) { - if (policy != return_value_policy::copy) - policy = return_value_policy::move; - return cast(&src, policy, parent); + static handle cast(itype &&src, return_value_policy, handle parent) { + return cast(&src, return_value_policy::move, parent); } static handle cast(const itype *src, return_value_policy policy, handle parent) { diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index b3b97cd80..840da043a 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -131,9 +131,14 @@ protected: capture *cap = (capture *) (sizeof(capture) <= sizeof(rec->data) ? &rec->data : rec->data[0]); + /* Override policy for rvalues -- always move */ + constexpr auto is_rvalue = !std::is_pointer::value + && !std::is_lvalue_reference::value; + const auto policy = is_rvalue ? return_value_policy::move : rec->policy; + /* Perform the function call */ handle result = cast_out::cast(args_converter.template call(cap->f), - rec->policy, parent); + policy, parent); /* Invoke call policy post-call hook */ detail::process_attributes::postcall(args, result); diff --git a/tests/test_python_types.cpp b/tests/test_python_types.cpp index 05ba4af9e..30fa3c795 100644 --- a/tests/test_python_types.cpp +++ b/tests/test_python_types.cpp @@ -164,6 +164,13 @@ public: int ExamplePythonTypes::value = 0; const int ExamplePythonTypes::value2 = 5; +struct MoveOutContainer { + struct Value { int value; }; + + std::list move_list() const { return {{0}, {1}, {2}}; } +}; + + test_initializer python_types([](py::module &m) { /* No constructor is explicitly defined below. An exception is raised when trying to construct it directly from Python */ @@ -363,4 +370,11 @@ test_initializer python_types([](py::module &m) { "memoryview"_a=d["memoryview"].cast() ); }); + + py::class_(m, "MoveOutContainerValue") + .def_readonly("value", &MoveOutContainer::Value::value); + + py::class_(m, "MoveOutContainer") + .def(py::init<>()) + .def_property_readonly("move_list", &MoveOutContainer::move_list); }); diff --git a/tests/test_python_types.py b/tests/test_python_types.py index 0b24c138f..c29679286 100644 --- a/tests/test_python_types.py +++ b/tests/test_python_types.py @@ -356,3 +356,15 @@ def test_constructors(): expected = {k.__name__: k(v) for k, v in data.items()} assert test_converting_constructors(inputs) == expected assert test_cast_functions(inputs) == expected + + +def test_move_out_container(): + """Properties use the `reference_internal` policy by default. If the underlying function + returns an rvalue, the policy is automatically changed to `move` to avoid referencing + a temporary. In case the return value is a container of user-defined types, the policy + also needs to be applied to the elements, not just the container.""" + from pybind11_tests import MoveOutContainer + + c = MoveOutContainer() + moved_out_list = c.move_list + assert [x.value for x in moved_out_list] == [0, 1, 2] diff --git a/tools/libsize.py b/tools/libsize.py index 70b811d4d..5dcb8b0d0 100644 --- a/tools/libsize.py +++ b/tools/libsize.py @@ -1,4 +1,4 @@ -from __future__ import print_function +from __future__ import print_function, division import os import sys