From a1be85f1e3e5124f2f465c4dd7f34cb6ad480752 Mon Sep 17 00:00:00 2001 From: Wenzel Jakob Date: Tue, 17 Jul 2018 16:56:26 +0200 Subject: [PATCH] stl.h: propagate return value policies to type-specific casters (#1455) * stl.h: propagate return value policies to type-specific casters Return value policies for containers like those handled in in 'stl.h' are currently broken. The problem is that detail::return_value_policy_override::policy() always returns 'move' when given a non-pointer/reference type, e.g. 'std::vector<...>'. This is sensible behavior for custom types that are exposed via 'py::class_<>', but it does not make sense for types that are handled by other type casters (STL containers, Eigen matrices, etc.). This commit changes the behavior so that detail::return_value_policy_override only becomes active when the type caster derives from type_caster_generic. Furthermore, the override logic is called recursively in STL type casters to enable key/value-specific behavior. --- include/pybind11/cast.h | 8 ++++++-- include/pybind11/eigen.h | 8 -------- include/pybind11/pybind11.h | 2 +- include/pybind11/stl.h | 10 ++++++++-- tests/test_stl.cpp | 18 ++++++++++++++++++ tests/test_stl.py | 10 ++++++++++ 6 files changed, 43 insertions(+), 13 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 4f664b064..214545083 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1561,9 +1561,13 @@ template using cast_is_temporary_value_reference = bool_constant // When a value returned from a C++ function is being cast back to Python, we almost always want to // force `policy = move`, regardless of the return value policy the function/method was declared -// with. Some classes (most notably Eigen::Ref and related) need to avoid this, and so can do so by -// specializing this struct. +// with. template struct return_value_policy_override { + static return_value_policy policy(return_value_policy p) { return p; } +}; + +template struct return_value_policy_override>::value, void>> { static return_value_policy policy(return_value_policy p) { return !std::is_lvalue_reference::value && !std::is_pointer::value ? return_value_policy::move : p; diff --git a/include/pybind11/eigen.h b/include/pybind11/eigen.h index 693a484dc..0899ec73f 100644 --- a/include/pybind11/eigen.h +++ b/include/pybind11/eigen.h @@ -350,14 +350,6 @@ private: Type value; }; -// Eigen Ref/Map classes have slightly different policy requirements, meaning we don't want to force -// `move` when a Ref/Map rvalue is returned; we treat Ref<> sort of like a pointer (we care about -// the underlying data, not the outer shell). -template -struct return_value_policy_override::value>> { - static return_value_policy policy(return_value_policy p) { return p; } -}; - // Base class for casting reference/map/block/etc. objects back to python. template struct eigen_map_caster { private: diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 8e47982e8..9094fc424 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -145,7 +145,7 @@ protected: capture *cap = const_cast(reinterpret_cast(data)); /* Override policy for rvalues -- usually to enforce rvp::move on an rvalue */ - const auto policy = return_value_policy_override::policy(call.func.policy); + return_value_policy policy = return_value_policy_override::policy(call.func.policy); /* Function scope guard -- defaults to the compile-to-nothing `void_type` */ using Guard = extract_guard_t; diff --git a/include/pybind11/stl.h b/include/pybind11/stl.h index 90eb7ea2e..1a4bbf0db 100644 --- a/include/pybind11/stl.h +++ b/include/pybind11/stl.h @@ -83,6 +83,7 @@ template struct set_caster { template static handle cast(T &&src, return_value_policy policy, handle parent) { + policy = return_value_policy_override::policy(policy); pybind11::set s; for (auto &&value : src) { auto value_ = reinterpret_steal(key_conv::cast(forward_like(value), policy, parent)); @@ -118,9 +119,11 @@ template struct map_caster { template static handle cast(T &&src, return_value_policy policy, handle parent) { dict d; + return_value_policy policy_key = return_value_policy_override::policy(policy); + return_value_policy policy_value = return_value_policy_override::policy(policy); for (auto &&kv : src) { - auto key = reinterpret_steal(key_conv::cast(forward_like(kv.first), policy, parent)); - auto value = reinterpret_steal(value_conv::cast(forward_like(kv.second), policy, parent)); + auto key = reinterpret_steal(key_conv::cast(forward_like(kv.first), policy_key, parent)); + auto value = reinterpret_steal(value_conv::cast(forward_like(kv.second), policy_value, parent)); if (!key || !value) return handle(); d[key] = value; @@ -158,6 +161,7 @@ private: public: template static handle cast(T &&src, return_value_policy policy, handle parent) { + policy = return_value_policy_override::policy(policy); list l(src.size()); size_t index = 0; for (auto &&value : src) { @@ -252,6 +256,7 @@ template struct optional_caster { static handle cast(T_ &&src, return_value_policy policy, handle parent) { if (!src) return none().inc_ref(); + policy = return_value_policy_override::policy(policy); return value_conv::cast(*std::forward(src), policy, parent); } @@ -356,6 +361,7 @@ struct variant_caster> { template struct type_caster> : variant_caster> { }; #endif + NAMESPACE_END(detail) inline std::ostream &operator<<(std::ostream &os, const handle &obj) { diff --git a/tests/test_stl.cpp b/tests/test_stl.cpp index 7d53e9c18..cd0985d02 100644 --- a/tests/test_stl.cpp +++ b/tests/test_stl.cpp @@ -8,6 +8,7 @@ */ #include "pybind11_tests.h" +#include "constructor_stats.h" #include // Test with `std::variant` in C++17 mode, or with `boost::variant` in C++11/14 @@ -235,4 +236,21 @@ TEST_SUBMODULE(stl, m) { // test_stl_pass_by_pointer m.def("stl_pass_by_pointer", [](std::vector* v) { return *v; }, "v"_a=nullptr); + + class Placeholder { + public: + Placeholder() { print_created(this); } + Placeholder(const Placeholder &) = delete; + ~Placeholder() { print_destroyed(this); } + }; + py::class_(m, "Placeholder"); + + /// test_stl_vector_ownership + m.def("test_stl_ownership", + []() { + std::vector result; + result.push_back(new Placeholder()); + return result; + }, + py::return_value_policy::take_ownership); } diff --git a/tests/test_stl.py b/tests/test_stl.py index fbf95ff06..99df6895a 100644 --- a/tests/test_stl.py +++ b/tests/test_stl.py @@ -2,6 +2,7 @@ import pytest from pybind11_tests import stl as m from pybind11_tests import UserType +from pybind11_tests import ConstructorStats def test_vector(doc): @@ -198,3 +199,12 @@ def test_missing_header_message(): with pytest.raises(TypeError) as excinfo: cm.missing_header_return() assert expected_message in str(excinfo.value) + + +def test_stl_ownership(): + cstats = ConstructorStats.get(m.Placeholder) + assert cstats.alive() == 0 + r = m.test_stl_ownership() + assert len(r) == 1 + del r + assert cstats.alive() == 0