From a007f062909223629d272c4bc2b0041809cac636 Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Sat, 6 Jan 2018 19:39:24 -0500 Subject: [PATCH 01/12] deregister_instance_impl: Fix #1238 --- include/pybind11/detail/class.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 7a5dd0130d..161ddb18e8 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -222,7 +222,7 @@ inline bool deregister_instance_impl(void *ptr, instance *self) { auto ®istered_instances = get_internals().registered_instances; auto range = registered_instances.equal_range(ptr); for (auto it = range.first; it != range.second; ++it) { - if (Py_TYPE(self) == Py_TYPE(it->second)) { + if (self == it->second && Py_TYPE(self) == Py_TYPE(it->second)) { registered_instances.erase(it); return true; } From 50d96b4eaede132ad0aa3386f650c72b652558ad Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Sat, 6 Jan 2018 14:02:31 -0500 Subject: [PATCH 02/12] Explicitly connect smart pointers and holders in the documentation. --- docs/advanced/smart_ptrs.rst | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/docs/advanced/smart_ptrs.rst b/docs/advanced/smart_ptrs.rst index da57748ca5..b695873934 100644 --- a/docs/advanced/smart_ptrs.rst +++ b/docs/advanced/smart_ptrs.rst @@ -1,5 +1,19 @@ -Smart pointers -############## +.. _holders: + +Smart pointers and holders +########################## + +Holders +======= + +The binding generator for classes, :class:`class_`, can be passed a template +type that denotes a special *holder* type that is used to manage references to +the object. If no such holder type template argument is given, the default for +a type named ``Type`` is ``std::unique_ptr``, which means that the object +is deallocated when Python's reference count goes to zero. It is possible to +switch to other types of reference counting wrappers or smart pointers, which +is useful in codebases that rely on them, such as ``std::shared_ptr``, or +even a custom type. std::unique_ptr =============== @@ -31,15 +45,10 @@ instance, the object might be referenced elsewhere). std::shared_ptr =============== -The binding generator for classes, :class:`class_`, can be passed a template -type that denotes a special *holder* type that is used to manage references to -the object. If no such holder type template argument is given, the default for -a type named ``Type`` is ``std::unique_ptr``, which means that the object -is deallocated when Python's reference count goes to zero. +If you have an existing code base with ``std::shared_ptr``, or you wish to enable +reference counting in C++ as well, then you may use this type as a holder. -It is possible to switch to other types of reference counting wrappers or smart -pointers, which is useful in codebases that rely on them. For instance, the -following snippet causes ``std::shared_ptr`` to be used instead. +As an example, the following snippet causes ``std::shared_ptr`` to be used instead. .. code-block:: cpp From ceb0e9651a346ed97f369a3a29a30137c5a56d6b Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Sat, 6 Jan 2018 19:46:15 -0500 Subject: [PATCH 03/12] cast: Clarify existing naming. --- include/pybind11/cast.h | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index efcdc5bac7..c891de396d 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -511,22 +511,22 @@ class type_caster_generic { } } - auto inst = reinterpret_steal(make_new_instance(tinfo->type)); - auto wrapper = reinterpret_cast(inst.ptr()); - wrapper->owned = false; - void *&valueptr = values_and_holders(wrapper).begin()->value_ptr(); + auto obj = reinterpret_steal(make_new_instance(tinfo->type)); + auto inst = reinterpret_cast(obj.ptr()); + inst->owned = false; + void *&valueptr = values_and_holders(inst).begin()->value_ptr(); switch (policy) { case return_value_policy::automatic: case return_value_policy::take_ownership: valueptr = src; - wrapper->owned = true; + inst->owned = true; break; case return_value_policy::automatic_reference: case return_value_policy::reference: valueptr = src; - wrapper->owned = false; + inst->owned = false; break; case return_value_policy::copy: @@ -535,7 +535,7 @@ class type_caster_generic { else throw cast_error("return_value_policy = copy, but the " "object is non-copyable!"); - wrapper->owned = true; + inst->owned = true; break; case return_value_policy::move: @@ -546,22 +546,22 @@ class type_caster_generic { else throw cast_error("return_value_policy = move, but the " "object is neither movable nor copyable!"); - wrapper->owned = true; + inst->owned = true; break; case return_value_policy::reference_internal: valueptr = src; - wrapper->owned = false; - keep_alive_impl(inst, parent); + inst->owned = false; + keep_alive_impl(obj, parent); break; default: throw cast_error("unhandled return_value_policy: should not happen!"); } - tinfo->init_instance(wrapper, existing_holder); + tinfo->init_instance(inst, existing_holder); - return inst.release(); + return obj.release(); } // Base methods for generic caster; there are overridden in copyable_holder_caster From b6296c2a4bdf8b2dfaaa9166037a0571762f5d52 Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Sat, 6 Jan 2018 13:45:40 -0500 Subject: [PATCH 04/12] Simplify predicate logic for `detail::move_always`, `detail::move_if_unreferenced`, and ensure that unqiue_ptr is accommodated. --- include/pybind11/cast.h | 31 ++++++++++++++++++++++--------- tests/test_smart_ptr.cpp | 14 ++++++++++++++ 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index c891de396d..3e67730265 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1495,6 +1495,15 @@ struct move_only_holder_caster { auto *ptr = holder_helper::get(src); return type_caster_base::cast_holder(ptr, &src); } + + // Force rvalue. + template + using cast_op_type = holder_type&&; + + operator holder_type&&() { + throw std::runtime_error("Currently unsupported"); + } + static constexpr auto name = type_caster_base::name; }; @@ -1565,21 +1574,25 @@ class type_caster::value>> : public pyobject_caste template using move_is_plain_type = satisfies_none_of; -template struct move_always : std::false_type {}; -template struct move_always struct move_common : std::false_type {}; +template struct move_common, - negation>, std::is_move_constructible, - std::is_same>().operator T&()), T&> + std::is_same< + intrinsic_t::template cast_op_type>, + T> +>::value>> : std::true_type {}; +template struct move_always : std::false_type {}; +template struct move_always, + negation> >::value>> : std::true_type {}; template struct move_if_unreferenced : std::false_type {}; template struct move_if_unreferenced, - negation>, - std::is_move_constructible, - std::is_same>().operator T&()), T&> + move_common, + is_copy_constructible >::value>> : std::true_type {}; -template using move_never = none_of, move_if_unreferenced>; +template using move_never = negation>; // Detect whether returning a `type` from a cast on type's type_caster is going to result in a // reference or pointer to a local variable of the type_caster. Basically, only diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 5f29850649..07402a237d 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -271,4 +271,18 @@ TEST_SUBMODULE(smart_ptr, m) { list.append(py::cast(e)); return list; }); + + // At present, only used for trait checks below. In the future, will be exposed to pybind. + struct UniquePtrHeld {}; + + // Check traits in a concise manner. + static_assert( + py::detail::move_common>::value, + "This trait must be true."); + static_assert( + py::detail::move_always>::value, + "This trait must be true."); + static_assert( + !py::detail::move_if_unreferenced>::value, + "This trait must be false."); } From 9e8a4f52d8832a76f8cf96d1527a0e92dc2f7670 Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Sat, 6 Jan 2018 13:46:05 -0500 Subject: [PATCH 05/12] Ensure that perfect forwarding is observed in cpp_function lambda wrappers. --- include/pybind11/pybind11.h | 4 ++-- tests/test_methods_and_attributes.py | 13 +++++++------ 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index a04d5365a0..aaa15eb3f9 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -70,14 +70,14 @@ class cpp_function : public function { /// Construct a cpp_function from a class method (non-const) template cpp_function(Return (Class::*f)(Arg...), const Extra&... extra) { - initialize([f](Class *c, Arg... args) -> Return { return (c->*f)(args...); }, + initialize([f](Class *c, Arg... args) -> Return { return (c->*f)(std::forward(args)...); }, (Return (*) (Class *, Arg...)) nullptr, extra...); } /// Construct a cpp_function from a class method (const) template cpp_function(Return (Class::*f)(Arg...) const, const Extra&... extra) { - initialize([f](const Class *c, Arg... args) -> Return { return (c->*f)(args...); }, + initialize([f](const Class *c, Arg... args) -> Return { return (c->*f)(std::forward(args)...); }, (Return (*)(const Class *, Arg ...)) nullptr, extra...); } diff --git a/tests/test_methods_and_attributes.py b/tests/test_methods_and_attributes.py index 86b2c3b4bb..4f7d310f62 100644 --- a/tests/test_methods_and_attributes.py +++ b/tests/test_methods_and_attributes.py @@ -4,10 +4,10 @@ def test_methods_and_attributes(): - instance1 = m.ExampleMandA() - instance2 = m.ExampleMandA(32) + instance1 = m.ExampleMandA() # 1 ctor + instance2 = m.ExampleMandA(32) # 1 ctor - instance1.add1(instance2) + instance1.add1(instance2) # 1 copy ctor + 1 move instance1.add2(instance2) instance1.add3(instance2) instance1.add4(instance2) @@ -20,7 +20,7 @@ def test_methods_and_attributes(): assert str(instance1) == "ExampleMandA[value=320]" assert str(instance2) == "ExampleMandA[value=32]" - assert str(instance1.self1()) == "ExampleMandA[value=320]" + assert str(instance1.self1()) == "ExampleMandA[value=320]" # 1 copy ctor + 1 move assert str(instance1.self2()) == "ExampleMandA[value=320]" assert str(instance1.self3()) == "ExampleMandA[value=320]" assert str(instance1.self4()) == "ExampleMandA[value=320]" @@ -58,8 +58,9 @@ def test_methods_and_attributes(): assert cstats.alive() == 0 assert cstats.values() == ["32"] assert cstats.default_constructions == 1 - assert cstats.copy_constructions == 3 - assert cstats.move_constructions >= 1 + assert cstats.copy_constructions == 2 + # On most platforms, there should be 2 moves. VisualStudio builds count 3. + assert cstats.move_constructions == 2 or cstats.move_constructions == 3 assert cstats.copy_assignments == 0 assert cstats.move_assignments == 0 From c240eb2ec89cd6cfab85bd6be68cf97275bb2a3a Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Sat, 6 Jan 2018 19:45:39 -0500 Subject: [PATCH 06/12] Enabling casting to unique_ptr. --- docs/advanced/smart_ptrs.rst | 27 +++++-- include/pybind11/attr.h | 3 + include/pybind11/cast.h | 108 +++++++++++++++++++++++----- include/pybind11/detail/internals.h | 9 +++ include/pybind11/pybind11.h | 26 ++++++- tests/test_smart_ptr.cpp | 55 +++++++++++++- tests/test_smart_ptr.py | 32 +++++++++ 7 files changed, 233 insertions(+), 27 deletions(-) diff --git a/docs/advanced/smart_ptrs.rst b/docs/advanced/smart_ptrs.rst index b695873934..f8f062289e 100644 --- a/docs/advanced/smart_ptrs.rst +++ b/docs/advanced/smart_ptrs.rst @@ -29,18 +29,31 @@ instances wrapped in C++11 unique pointers, like so m.def("create_example", &create_example); -In other words, there is nothing special that needs to be done. While returning -unique pointers in this way is allowed, it is *illegal* to use them as function -arguments. For instance, the following function signature cannot be processed -by pybind11. +In other words, there is nothing special that needs to be done. Also note that +you may use ``std::unique_ptr`` as an argument to a function (or as a type in +``py::move`` / ``py::cast``): .. code-block:: cpp void do_something_with_example(std::unique_ptr ex) { ... } -The above signature would imply that Python needs to give up ownership of an -object that is passed to this function, which is generally not possible (for -instance, the object might be referenced elsewhere). +When a pybind object is passed to this function signature, please note that +pybind will no longer have ownership of this object (meaning C++ may destroy +the object while there are still existing Python references). Care must be +taken, the same as what is done for bare pointers. + +In the above function, note that the lifetime of this object is *terminal*, +meaning that Python should *not* refer to the object after the function is done +calling. You *may* return ownership back to pybind by casting the object, as so: + +.. code-block:: cpp + + void do_something_with_example(std::unique_ptr ex) { + // ... operations... + py::cast(std::move(ex)); // This gives pybind back ownership. + } + +If this is done, then you may continue referencing the object in Python. std::shared_ptr =============== diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index dce875a6b9..ff0e3bbcfa 100644 --- a/include/pybind11/attr.h +++ b/include/pybind11/attr.h @@ -250,6 +250,9 @@ struct type_record { /// Is the class definition local to the module shared object? bool module_local : 1; + /* See `type_info::ownership_info_t` for more information.) */ + type_info::ownership_info_t ownership_info; + PYBIND11_NOINLINE void add_base(const std::type_info &base, void *(*caster)(void *)) { auto base_info = detail::get_type_info(base, false); if (!base_info) { diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 3e67730265..3a0eedb4c4 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -479,6 +479,20 @@ inline PyThreadState *get_thread_state_unchecked() { inline void keep_alive_impl(handle nurse, handle patient); inline PyObject *make_new_instance(PyTypeObject *type); +inline bool reclaim_existing_if_needed( + instance *inst, const detail::type_info *tinfo, const void *existing_holder) { + // Only reclaim if (a) we have an existing holder and (b) if it's a move-only holder. + // TODO: Remove `default_holder`, store more descriptive holder information. + if (existing_holder && tinfo->default_holder) { + // Requesting reclaim from C++. + value_and_holder v_h = inst->get_value_and_holder(tinfo); + // TODO(eric.cousineau): Add `holder_type_erased` to avoid need for `const_cast`. + tinfo->ownership_info.reclaim(v_h, const_cast(existing_holder)); + return true; + } + return false; +} + class type_caster_generic { public: PYBIND11_NOINLINE type_caster_generic(const std::type_info &type_info) @@ -506,8 +520,12 @@ class type_caster_generic { auto it_instances = get_internals().registered_instances.equal_range(src); for (auto it_i = it_instances.first; it_i != it_instances.second; ++it_i) { for (auto instance_type : detail::all_type_info(Py_TYPE(it_i->second))) { - if (instance_type && same_type(*instance_type->cpptype, *tinfo->cpptype)) - return handle((PyObject *) it_i->second).inc_ref(); + if (instance_type && same_type(*instance_type->cpptype, *tinfo->cpptype)) { + // Casting for an already registered type. Return existing reference. + instance *inst = it_i->second; + reclaim_existing_if_needed(inst, tinfo, existing_holder); + return handle((PyObject *) inst).inc_ref(); + } } } @@ -1405,6 +1423,16 @@ struct holder_helper { static auto get(const T &p) -> decltype(p.get()) { return p.get(); } }; +template +cast_error cast_error_holder_unheld() { + return cast_error("Unable to cast from non-held to held instance (T& to Holder) " +#if defined(NDEBUG) + "(compile in debug mode for type information)"); +#else + "of type '" + type_id() + "''"); +#endif +} + /// Type caster for holder types like std::shared_ptr, etc. template struct copyable_holder_caster : public type_caster_base { @@ -1451,12 +1479,7 @@ struct copyable_holder_caster : public type_caster_base { holder = v_h.template holder(); return true; } else { - throw cast_error("Unable to cast from non-held to held instance (T& to Holder) " -#if defined(NDEBUG) - "(compile in debug mode for type information)"); -#else - "of type '" + type_id() + "''"); -#endif + throw cast_error_holder_unheld(); } } @@ -1478,7 +1501,7 @@ struct copyable_holder_caster : public type_caster_base { static bool try_direct_conversions(handle) { return false; } - +private: holder_type holder; }; @@ -1487,13 +1510,17 @@ template class type_caster> : public copyable_holder_caster> { }; template -struct move_only_holder_caster { - static_assert(std::is_base_of, type_caster>::value, +struct move_only_holder_caster : type_caster_base { + using base = type_caster_base; + static_assert(std::is_base_of>::value, "Holder classes are only supported for custom types"); + using base::base; + using base::cast; + using base::typeinfo; + using base::value; - static handle cast(holder_type &&src, return_value_policy, handle) { - auto *ptr = holder_helper::get(src); - return type_caster_base::cast_holder(ptr, &src); + bool load(handle src, bool convert) { + return base::template load_impl>(src, convert); } // Force rvalue. @@ -1501,10 +1528,38 @@ struct move_only_holder_caster { using cast_op_type = holder_type&&; operator holder_type&&() { - throw std::runtime_error("Currently unsupported"); + return std::move(holder); + } + + static handle cast(holder_type &&src, return_value_policy, handle) { + auto *ptr = holder_helper::get(src); + handle h = type_caster_base::cast_holder(ptr, &src); + assert(src.get() == nullptr); + return h; + } + +protected: + friend class type_caster_generic; + void check_holder_compat() {} + + bool load_value(value_and_holder &&v_h) { + if (v_h.holder_constructed()) { + // Do NOT use `v_h.type`. + typeinfo->ownership_info.release(v_h, &holder); + assert(v_h.holder().get() == nullptr); + return true; + } else { + throw cast_error_holder_unheld(); + } } - static constexpr auto name = type_caster_base::name; + // TODO(eric.cousineau): Resolve this. + bool try_implicit_casts(handle, bool) { return false; } + + static bool try_direct_conversions(handle) { return false; } + +private: + holder_type holder; }; template @@ -1662,6 +1717,25 @@ object cast(const T &value, return_value_policy policy = return_value_policy::au template T handle::cast() const { return pybind11::cast(*this); } template <> inline void handle::cast() const { return; } +template +detail::enable_if_t< + // TODO(eric.cousineau): Figure out how to prevent perfect-forwarding more elegantly. + std::is_rvalue_reference::value && !detail::is_pyobject>::value, object> + move(T&& value) { + // TODO(eric.cousineau): Should the user be able to specify policies / parent? + handle no_parent; + return reinterpret_steal( + detail::make_caster::cast(std::move(value), return_value_policy::take_ownership, no_parent)); +} + +template +detail::enable_if_t< + std::is_rvalue_reference::value && !detail::is_pyobject>::value, object> + cast(T&& value) { + // Have to use `pybind11::move` because some compilers might try to bind `move` to `std::move`... + return pybind11::move(std::move(value)); +} + template detail::enable_if_t::value, T> move(object &&obj) { if (obj.ref_count() > 1) @@ -1674,7 +1748,7 @@ detail::enable_if_t::value, T> move(object &&obj) { #endif // Move into a temporary and return that, because the reference may be a local value of `conv` - T ret = std::move(detail::load_type(obj).operator T&()); + T ret = std::move(detail::cast_op(detail::load_type(obj))); return ret; } diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index e39f38695f..1a74b348cb 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -108,6 +108,15 @@ struct type_info { bool default_holder : 1; /* true if this is a type registered with py::module_local */ bool module_local : 1; + /* Holder information. */ + struct ownership_info_t { + typedef void (*transfer_t)(detail::value_and_holder& v_h, void* existing_holder_raw); + // Release an instance to C++. + transfer_t release = nullptr; + // Reclaim an instance from C++. + transfer_t reclaim = nullptr; + }; + ownership_info_t ownership_info; }; /// Tracks the `internals` and `type_info` ABI version independent of the main library version diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index aaa15eb3f9..72fd1d8b60 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -899,6 +899,7 @@ class generic_type : public object { tinfo->simple_ancestors = true; tinfo->default_holder = rec.default_holder; tinfo->module_local = rec.module_local; + tinfo->ownership_info = rec.ownership_info; auto &internals = get_internals(); auto tindex = std::type_index(*rec.type); @@ -1053,7 +1054,8 @@ class class_ : public detail::generic_type { record.init_instance = init_instance; record.dealloc = dealloc; record.default_holder = std::is_same>::value; - + record.ownership_info.release = holder_release; + record.ownership_info.reclaim = holder_reclaim; set_operator_new(&record); /* Register base classes specified via template arguments to class_, if any */ @@ -1070,6 +1072,28 @@ class class_ : public detail::generic_type { } } + static void holder_release(detail::value_and_holder& v_h, void* external_holder_raw) { + // Release from `v_h.holder<...>()` into `external_holder`. + assert(v_h.inst->owned && v_h.holder_constructed() && "Internal error: Object must be owned"); + assert(external_holder_raw && "Internal error: External holder must not be null"); + holder_type& holder = v_h.holder(); + holder_type& external_holder = *reinterpret_cast(external_holder_raw); + external_holder = std::move(holder); + holder.~holder_type(); + v_h.set_holder_constructed(false); + v_h.inst->owned = false; + } + + static void holder_reclaim(detail::value_and_holder& v_h, void* external_holder_raw) { + // Reclaim from `external_holder` into `v_h.holder<...>()`. + assert(!v_h.inst->owned && !v_h.holder_constructed() && "Internal error: Object must not be owned"); + assert(external_holder_raw && "Internal error: External holder must not be null"); + holder_type& external_holder = *reinterpret_cast(external_holder_raw); + new (&v_h.holder()) holder_type(std::move(external_holder)); + v_h.set_holder_constructed(); + v_h.inst->owned = true; + } + template ::value, int> = 0> static void add_base(detail::type_record &rec) { rec.add_base(typeid(Base), [](void *src) -> void * { diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 07402a237d..1f511f136c 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -272,8 +272,23 @@ TEST_SUBMODULE(smart_ptr, m) { return list; }); - // At present, only used for trait checks below. In the future, will be exposed to pybind. - struct UniquePtrHeld {}; + class UniquePtrHeld { + public: + UniquePtrHeld() = delete; + UniquePtrHeld(const UniquePtrHeld&) = delete; + UniquePtrHeld(UniquePtrHeld&&) = delete; + + UniquePtrHeld(int value) + : value_(value) { + print_created(this, value); + } + ~UniquePtrHeld() { + print_destroyed(this); + } + int value() const { return value_; } + private: + int value_{}; + }; // Check traits in a concise manner. static_assert( @@ -285,4 +300,40 @@ TEST_SUBMODULE(smart_ptr, m) { static_assert( !py::detail::move_if_unreferenced>::value, "This trait must be false."); + + py::class_(m, "UniquePtrHeld") + .def(py::init()) + .def("value", &UniquePtrHeld::value); + + m.def("unique_ptr_pass_through", + [](std::unique_ptr obj) { + return obj; + }); + m.def("unique_ptr_terminal", + [](std::unique_ptr obj) { + obj.reset(); + return nullptr; + }); + + // Guarantee API works as expected. + m.def("unique_ptr_pass_through_cast_from_py", + [](py::object obj_py) { + auto obj = + py::cast>(std::move(obj_py)); + return obj; + }); + m.def("unique_ptr_pass_through_move_from_py", + [](py::object obj_py) { + return py::move>(std::move(obj_py)); + }); + + m.def("unique_ptr_pass_through_move_to_py", + [](std::unique_ptr obj) { + return py::move(std::move(obj)); + }); + + m.def("unique_ptr_pass_through_cast_to_py", + [](std::unique_ptr obj) { + return py::cast(std::move(obj)); + }); } diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index 4dfe0036fc..5f1e20b967 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -218,3 +218,35 @@ def test_shared_ptr_gc(): pytest.gc_collect() for i, v in enumerate(el.get()): assert i == v.value() + + +def test_unique_ptr_arg(): + stats = ConstructorStats.get(m.UniquePtrHeld) + + pass_through_list = [ + m.unique_ptr_pass_through, + m.unique_ptr_pass_through_cast_from_py, + m.unique_ptr_pass_through_move_from_py, + m.unique_ptr_pass_through_move_to_py, + m.unique_ptr_pass_through_cast_to_py, + ] + for pass_through in pass_through_list: + obj = m.UniquePtrHeld(1) + obj_ref = m.unique_ptr_pass_through(obj) + assert stats.alive() == 1 + assert obj.value() == 1 + assert obj == obj_ref + del obj + del obj_ref + pytest.gc_collect() + assert stats.alive() == 0 + + obj = m.UniquePtrHeld(1) + m.unique_ptr_terminal(obj) + assert stats.alive() == 0 + + m.unique_ptr_terminal(m.UniquePtrHeld(2)) + assert stats.alive() == 0 + + assert m.unique_ptr_pass_through(None) is None + m.unique_ptr_terminal(None) From 832d466317f943613c9fdc6fdee9d2a81dd0bef6 Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Sat, 6 Jan 2018 19:57:04 -0500 Subject: [PATCH 07/12] Clear existing patients when ownership is reclaimed by pybind. --- docs/advanced/smart_ptrs.rst | 4 +++ include/pybind11/detail/class.h | 2 ++ include/pybind11/pybind11.h | 3 ++ tests/test_smart_ptr.cpp | 47 ++++++++++++++++++++++++ tests/test_smart_ptr.py | 64 +++++++++++++++++++++++++++++++++ 5 files changed, 120 insertions(+) diff --git a/docs/advanced/smart_ptrs.rst b/docs/advanced/smart_ptrs.rst index f8f062289e..155f710603 100644 --- a/docs/advanced/smart_ptrs.rst +++ b/docs/advanced/smart_ptrs.rst @@ -55,6 +55,10 @@ calling. You *may* return ownership back to pybind by casting the object, as so: If this is done, then you may continue referencing the object in Python. +When Pybind regains ownership of a Python object, it will detach any existing +``keep_alive`` behavior, since this is commonly used for containers that +must be kept alive because they would destroy the object that they owned. + std::shared_ptr =============== diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 161ddb18e8..985a2be0fe 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -296,6 +296,8 @@ inline void add_patient(PyObject *nurse, PyObject *patient) { inline void clear_patients(PyObject *self) { auto instance = reinterpret_cast(self); + if (!instance->has_patients) + return; auto &internals = get_internals(); auto pos = internals.patients.find(self); assert(pos != internals.patients.end()); diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 72fd1d8b60..fb4d6808c7 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1092,6 +1092,9 @@ class class_ : public detail::generic_type { new (&v_h.holder()) holder_type(std::move(external_holder)); v_h.set_holder_constructed(); v_h.inst->owned = true; + // If this instance is now owend by pybind, release any existing + // patients (owners for `reference_internal`). + detail::clear_patients((PyObject*)v_h.inst); } template ::value, int> = 0> diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 1f511f136c..6b35b2f74b 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -56,6 +56,48 @@ class custom_unique_ptr { PYBIND11_DECLARE_HOLDER_TYPE(T, custom_unique_ptr); +enum class KeepAliveType : int { + Plain = 0, + KeepAlive, +}; + +template < + typename T, + KeepAliveType keep_alive_type> +class Container { +public: + using Ptr = std::unique_ptr; + Container(Ptr ptr) + : ptr_(std::move(ptr)) { + print_created(this); + } + ~Container() { + print_destroyed(this); + } + T* get() const { return ptr_.get(); } + Ptr release() { + return std::move(ptr_); + } + void reset(Ptr ptr) { + ptr_ = std::move(ptr); + } + + static void def(py::module &m, const std::string& name) { + py::class_ cls(m, name.c_str()); + if (keep_alive_type == KeepAliveType::KeepAlive) { + cls.def(py::init(), py::keep_alive<2, 1>()); + } else { + cls.def(py::init()); + } + // TODO: Figure out why reference_internal does not work??? + cls.def("get", &Container::get, py::keep_alive<0, 1>()); //py::return_value_policy::reference_internal); + cls.def("release", &Container::release); + cls.def("reset", &Container::reset); + } +private: + Ptr ptr_; +}; + TEST_SUBMODULE(smart_ptr, m) { // test_smart_ptr @@ -336,4 +378,9 @@ TEST_SUBMODULE(smart_ptr, m) { [](std::unique_ptr obj) { return py::cast(std::move(obj)); }); + + Container::def( + m, "ContainerPlain"); + Container::def( + m, "ContainerKeepAlive"); } diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index 5f1e20b967..31dae04dd3 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -1,4 +1,5 @@ import pytest +import weakref from pybind11_tests import smart_ptr as m from pybind11_tests import ConstructorStats @@ -250,3 +251,66 @@ def test_unique_ptr_arg(): assert m.unique_ptr_pass_through(None) is None m.unique_ptr_terminal(None) + + +def test_unique_ptr_keep_alive(): + obj_stats = ConstructorStats.get(m.UniquePtrHeld) + c_plain_stats = ConstructorStats.get(m.ContainerPlain) + c_keep_stats = ConstructorStats.get(m.ContainerKeepAlive) + + # Try with plain container. + obj = m.UniquePtrHeld(1) + c_plain = m.ContainerPlain(obj) + c_plain_wref = weakref.ref(c_plain) + assert obj_stats.alive() == 1 + assert c_plain_stats.alive() == 1 + del c_plain + pytest.gc_collect() + # Everything should have died. + assert c_plain_wref() is None + assert c_plain_stats.alive() == 0 + assert obj_stats.alive() == 0 + del obj + + # Ensure keep_alive via `reference_internal` still works. + obj = m.UniquePtrHeld(2) + c_plain = m.ContainerPlain(obj) + assert c_plain.get() is obj # Trigger keep_alive + assert obj_stats.alive() == 1 + assert c_plain_stats.alive() == 1 + del c_plain + pytest.gc_collect() + assert obj_stats.alive() == 1 + assert c_plain_stats.alive() == 1 + del obj + pytest.gc_collect() + assert obj_stats.alive() == 0 + assert c_plain_stats.alive() == 0 + + # Now try with keep-alive container. + # Primitive, very non-conservative. + obj = m.UniquePtrHeld(3) + c_keep = m.ContainerKeepAlive(obj) + c_keep_wref = weakref.ref(c_keep) + assert obj_stats.alive() == 1 + assert c_keep_stats.alive() == 1 + del c_keep + pytest.gc_collect() + # Everything should have stayed alive. + assert c_keep_wref() is not None + assert c_keep_stats.alive() == 1 + assert obj_stats.alive() == 1 + # Now release the object. This should have released the container as a patient. + c_keep_wref().release() + pytest.gc_collect() + assert obj_stats.alive() == 1 + assert c_keep_stats.alive() == 0 + + # Check with nullptr. + c_keep = m.ContainerKeepAlive(None) + assert c_keep_stats.alive() == 1 + obj = c_keep.get() + assert obj is None + del c_keep + pytest.gc_collect() + assert c_keep_stats.alive() == 0 From f12898ff83091b23c2857dd0015d15a14e8a2c26 Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Sat, 6 Jan 2018 20:40:25 -0500 Subject: [PATCH 08/12] Ensure that we can use `reference_internal` on existing instances. --- include/pybind11/cast.h | 52 ++++++++++++++++++++++++++------- include/pybind11/detail/class.h | 11 +++++++ include/pybind11/pybind11.h | 12 ++++++-- tests/test_smart_ptr.cpp | 3 +- 4 files changed, 63 insertions(+), 15 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 3a0eedb4c4..b5fde130f3 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -476,18 +476,50 @@ inline PyThreadState *get_thread_state_unchecked() { } // Forward declarations +inline bool has_patient(PyObject *nurse, PyObject *patient); inline void keep_alive_impl(handle nurse, handle patient); inline PyObject *make_new_instance(PyTypeObject *type); -inline bool reclaim_existing_if_needed( - instance *inst, const detail::type_info *tinfo, const void *existing_holder) { - // Only reclaim if (a) we have an existing holder and (b) if it's a move-only holder. - // TODO: Remove `default_holder`, store more descriptive holder information. - if (existing_holder && tinfo->default_holder) { - // Requesting reclaim from C++. - value_and_holder v_h = inst->get_value_and_holder(tinfo); - // TODO(eric.cousineau): Add `holder_type_erased` to avoid need for `const_cast`. - tinfo->ownership_info.reclaim(v_h, const_cast(existing_holder)); +inline void reclaim_instance( + instance *inst, const detail::type_info *tinfo, const void *existing_holder = nullptr) { + value_and_holder v_h = inst->get_value_and_holder(tinfo); + // TODO(eric.cousineau): Add `holder_type_erased` to avoid need for `const_cast`. + tinfo->ownership_info.reclaim(v_h, const_cast(existing_holder)); +} + +inline bool reclaim_instance_if_needed( + instance *inst, const detail::type_info *tinfo, const void *existing_holder, + return_value_policy policy, handle parent) { + // TODO(eric.cousineau): Remove `default_holder`, store more descriptive holder information. + // Only handle reclaim if it's a move-only holder, and not currently owned. + // Let copyable holders do their own thing. + if (!tinfo->default_holder || inst->owned) + // TODO(eric.cousineau): For shared ptrs, there was a point where the holder was constructed, + // but the instance was not owned. What does that mean? + return false; + bool do_reclaim = false; + if (existing_holder) { + // This means that we're coming from a holder caster. + do_reclaim = true; + } else { + // Check existing policies. + switch (policy) { + case return_value_policy::reference_internal: { + handle h = handle((PyObject *) inst); + if (!has_patient(h.ptr(), parent.ptr())) + keep_alive_impl(h, parent); + break; + } + case return_value_policy::take_ownership: { + do_reclaim = true; + break; + } + default: + break; + } + } + if (do_reclaim) { + reclaim_instance(inst, tinfo, existing_holder); return true; } return false; @@ -523,7 +555,7 @@ class type_caster_generic { if (instance_type && same_type(*instance_type->cpptype, *tinfo->cpptype)) { // Casting for an already registered type. Return existing reference. instance *inst = it_i->second; - reclaim_existing_if_needed(inst, tinfo, existing_holder); + reclaim_instance_if_needed(inst, tinfo, existing_holder, policy, parent); return handle((PyObject *) inst).inc_ref(); } } diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 985a2be0fe..9b3e1e5fcf 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -9,6 +9,8 @@ #pragma once +#include + #include "../attr.h" NAMESPACE_BEGIN(PYBIND11_NAMESPACE) @@ -294,6 +296,15 @@ inline void add_patient(PyObject *nurse, PyObject *patient) { internals.patients[nurse].push_back(patient); } +inline bool has_patient(PyObject *nurse, PyObject *patient) { + auto &internals = get_internals(); + auto instance = reinterpret_cast(nurse); + if (!instance->has_patients) + return false; + auto& cur = internals.patients[nurse]; + return (std::find(cur.begin(), cur.end(), patient) != cur.end()); +} + inline void clear_patients(PyObject *self) { auto instance = reinterpret_cast(self); if (!instance->has_patients) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index fb4d6808c7..28ea1f61da 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1087,9 +1087,15 @@ class class_ : public detail::generic_type { static void holder_reclaim(detail::value_and_holder& v_h, void* external_holder_raw) { // Reclaim from `external_holder` into `v_h.holder<...>()`. assert(!v_h.inst->owned && !v_h.holder_constructed() && "Internal error: Object must not be owned"); - assert(external_holder_raw && "Internal error: External holder must not be null"); - holder_type& external_holder = *reinterpret_cast(external_holder_raw); - new (&v_h.holder()) holder_type(std::move(external_holder)); + holder_type& holder = v_h.holder(); + if (external_holder_raw) { + // Take from external holder. + holder_type& external_holder = *reinterpret_cast(external_holder_raw); + new (&holder) holder_type(std::move(external_holder)); + } else { + // Construct new holder, using existing value. + new (&holder) holder_type(v_h.value_ptr()); + } v_h.set_holder_constructed(); v_h.inst->owned = true; // If this instance is now owend by pybind, release any existing diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 6b35b2f74b..30d8afc67c 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -89,8 +89,7 @@ class Container { } else { cls.def(py::init()); } - // TODO: Figure out why reference_internal does not work??? - cls.def("get", &Container::get, py::keep_alive<0, 1>()); //py::return_value_policy::reference_internal); + cls.def("get", &Container::get, py::return_value_policy::reference_internal); cls.def("release", &Container::release); cls.def("reset", &Container::reset); } From 9265f75ca1160756ce6d24590c5f05ed5831b011 Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Sat, 6 Jan 2018 21:41:42 -0500 Subject: [PATCH 09/12] Add derived class test for unique_ptr arguments. --- tests/test_smart_ptr.cpp | 20 +++++++++++++++++++- tests/test_smart_ptr.py | 11 +++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 30d8afc67c..627c1b4ea0 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -323,7 +323,7 @@ TEST_SUBMODULE(smart_ptr, m) { : value_(value) { print_created(this, value); } - ~UniquePtrHeld() { + virtual ~UniquePtrHeld() { print_destroyed(this); } int value() const { return value_; } @@ -382,4 +382,22 @@ TEST_SUBMODULE(smart_ptr, m) { m, "ContainerPlain"); Container::def( m, "ContainerKeepAlive"); + + class UniquePtrDerived : public UniquePtrHeld { + public: + UniquePtrDerived(int value, std::string name) + : UniquePtrHeld(value), name_(name) { + print_created(this, name); + } + ~UniquePtrDerived() { + print_destroyed(this); + } + std::string name() const { return name_; } + private: + std::string name_{}; + }; + + py::class_(m, "UniquePtrDerived") + .def(py::init()) + .def("name", &UniquePtrDerived::name); } diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index 31dae04dd3..78f549b133 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -314,3 +314,14 @@ def test_unique_ptr_keep_alive(): del c_keep pytest.gc_collect() assert c_keep_stats.alive() == 0 + + +def test_unique_ptr_derived(): + obj = m.UniquePtrDerived(1, "a") + c_plain = m.ContainerPlain(obj) + del obj + pytest.gc_collect() + obj = c_plain.release() + assert obj.value() == 1 + assert obj.name() == "a" + del obj From af34dd50c4cfb2073564a825c88d7bd815806cda Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Sun, 22 Apr 2018 21:59:06 -0400 Subject: [PATCH 10/12] unique_ptr: Ensure overloads work as desired --- tests/test_smart_ptr.cpp | 4 ++++ tests/test_smart_ptr.py | 3 +++ 2 files changed, 7 insertions(+) diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 627c1b4ea0..2a51de9304 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -346,6 +346,10 @@ TEST_SUBMODULE(smart_ptr, m) { .def(py::init()) .def("value", &UniquePtrHeld::value); + class UniquePtrOther {}; + py::class_(m, "UniquePtrOther") + .def(py::init<>()); + m.def("unique_ptr_pass_through", [](std::unique_ptr obj) { return obj; diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index 78f549b133..9e7f618cf3 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -252,6 +252,9 @@ def test_unique_ptr_arg(): assert m.unique_ptr_pass_through(None) is None m.unique_ptr_terminal(None) + with pytest.raises(TypeError): + m.unique_ptr_terminal(m.UniquePtrOther()) + def test_unique_ptr_keep_alive(): obj_stats = ConstructorStats.get(m.UniquePtrHeld) From d16342ccd0e01b40028eeef9fe4d267626813aa3 Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Sun, 22 Apr 2018 22:02:04 -0400 Subject: [PATCH 11/12] unique_ptr: Confirm RobotLocomotion/drake#8160 --- tests/test_smart_ptr.cpp | 22 ++++++++++++++++++++++ tests/test_smart_ptr.py | 11 +++++++++++ 2 files changed, 33 insertions(+) diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 2a51de9304..a301ef2975 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -404,4 +404,26 @@ TEST_SUBMODULE(smart_ptr, m) { py::class_(m, "UniquePtrDerived") .def(py::init()) .def("name", &UniquePtrDerived::name); + + class FirstT {}; + py::class_(m, "FirstT") + .def(py::init()); + class SecondT {}; + py::class_(m, "SecondT") + .def(py::init()); + + m.def("unique_ptr_overload", + [](std::unique_ptr obj, FirstT) { + py::dict out; + out["obj"] = py::cast(std::move(obj)); + out["overload"] = 1; + return out; + }); + m.def("unique_ptr_overload", + [](std::unique_ptr obj, SecondT) { + py::dict out; + out["obj"] = py::cast(std::move(obj)); + out["overload"] = 2; + return out; + }); } diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index 9e7f618cf3..a27810f326 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -328,3 +328,14 @@ def test_unique_ptr_derived(): assert obj.value() == 1 assert obj.name() == "a" del obj + + +def test_unique_ptr_overload_fail(): + obj = m.UniquePtrHeld(1) + # These overloads pass ownership back to Python. + out = m.unique_ptr_overload(obj, m.FirstT()) + assert out["obj"] is obj + assert out["overload"] == 1 + out = m.unique_ptr_overload(obj, m.SecondT()) + assert out["obj"] is obj + assert out["overload"] == 2 From a6dc1cc6b01cc854663e10f2ace25adef29088a3 Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Sun, 22 Apr 2018 22:03:53 -0400 Subject: [PATCH 12/12] unique_ptr: Resolve RobotLocomotion/drake#8160 --- include/pybind11/cast.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index b5fde130f3..569257faaa 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1551,6 +1551,22 @@ struct move_only_holder_caster : type_caster_base { using base::typeinfo; using base::value; + // We must explicitly define the default constructor(s) since we define a + // destructor; otherwise, the compiler will incorrectly use the copy + // constructor. + move_only_holder_caster() = default; + move_only_holder_caster(move_only_holder_caster&&) = default; + move_only_holder_caster(const move_only_holder_caster&) = delete; + ~move_only_holder_caster() { + if (holder) { + // If the argument was loaded into C++, but not transferred out, + // then this was most likely part of a failed overload in + // `argument_loader`. Transfer ownership back to Python. + move_only_holder_caster::cast( + std::move(holder), return_value_policy{}, handle{}); + } + } + bool load(handle src, bool convert) { return base::template load_impl>(src, convert); }