Skip to content

Commit a2ac6da

Browse files
Ensure that we can use reference_internal on existing instances.
1 parent 3e30168 commit a2ac6da

File tree

4 files changed

+63
-15
lines changed

4 files changed

+63
-15
lines changed

include/pybind11/cast.h

Lines changed: 42 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -476,18 +476,50 @@ inline PyThreadState *get_thread_state_unchecked() {
476476
}
477477

478478
// Forward declarations
479+
inline bool has_patient(PyObject *nurse, PyObject *patient);
479480
inline void keep_alive_impl(handle nurse, handle patient);
480481
inline PyObject *make_new_instance(PyTypeObject *type);
481482

482-
inline bool reclaim_existing_if_needed(
483-
instance *inst, const detail::type_info *tinfo, const void *existing_holder) {
484-
// Only reclaim if (a) we have an existing holder and (b) if it's a move-only holder.
485-
// TODO: Remove `default_holder`, store more descriptive holder information.
486-
if (existing_holder && tinfo->default_holder) {
487-
// Requesting reclaim from C++.
488-
value_and_holder v_h = inst->get_value_and_holder(tinfo);
489-
// TODO(eric.cousineau): Add `holder_type_erased` to avoid need for `const_cast`.
490-
tinfo->ownership_info.reclaim(v_h, const_cast<void*>(existing_holder));
483+
inline void reclaim_instance(
484+
instance *inst, const detail::type_info *tinfo, const void *existing_holder = nullptr) {
485+
value_and_holder v_h = inst->get_value_and_holder(tinfo);
486+
// TODO(eric.cousineau): Add `holder_type_erased` to avoid need for `const_cast`.
487+
tinfo->ownership_info.reclaim(v_h, const_cast<void*>(existing_holder));
488+
}
489+
490+
inline bool reclaim_instance_if_needed(
491+
instance *inst, const detail::type_info *tinfo, const void *existing_holder,
492+
return_value_policy policy, handle parent) {
493+
// TODO(eric.cousineau): Remove `default_holder`, store more descriptive holder information.
494+
// Only handle reclaim if it's a move-only holder, and not currently owned.
495+
// Let copyable holders do their own thing.
496+
if (!tinfo->default_holder || inst->owned)
497+
// TODO(eric.cousineau): For shared ptrs, there was a point where the holder was constructed,
498+
// but the instance was not owned. What does that mean?
499+
return false;
500+
bool do_reclaim = false;
501+
if (existing_holder) {
502+
// This means that we're coming from a holder caster.
503+
do_reclaim = true;
504+
} else {
505+
// Check existing policies.
506+
switch (policy) {
507+
case return_value_policy::reference_internal: {
508+
handle h = handle((PyObject *) inst);
509+
if (!has_patient(h.ptr(), parent.ptr()))
510+
keep_alive_impl(h, parent);
511+
break;
512+
}
513+
case return_value_policy::take_ownership: {
514+
do_reclaim = true;
515+
break;
516+
}
517+
default:
518+
break;
519+
}
520+
}
521+
if (do_reclaim) {
522+
reclaim_instance(inst, tinfo, existing_holder);
491523
return true;
492524
}
493525
return false;
@@ -523,7 +555,7 @@ class type_caster_generic {
523555
if (instance_type && same_type(*instance_type->cpptype, *tinfo->cpptype)) {
524556
// Casting for an already registered type. Return existing reference.
525557
instance *inst = it_i->second;
526-
reclaim_existing_if_needed(inst, tinfo, existing_holder);
558+
reclaim_instance_if_needed(inst, tinfo, existing_holder, policy, parent);
527559
return handle((PyObject *) inst).inc_ref();
528560
}
529561
}

include/pybind11/detail/class.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99

1010
#pragma once
1111

12+
#include <algorithm>
13+
1214
#include "../attr.h"
1315

1416
NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
@@ -294,6 +296,15 @@ inline void add_patient(PyObject *nurse, PyObject *patient) {
294296
internals.patients[nurse].push_back(patient);
295297
}
296298

299+
inline bool has_patient(PyObject *nurse, PyObject *patient) {
300+
auto &internals = get_internals();
301+
auto instance = reinterpret_cast<detail::instance *>(nurse);
302+
if (!instance->has_patients)
303+
return false;
304+
auto& cur = internals.patients[nurse];
305+
return (std::find(cur.begin(), cur.end(), patient) != cur.end());
306+
}
307+
297308
inline void clear_patients(PyObject *self) {
298309
auto instance = reinterpret_cast<detail::instance *>(self);
299310
if (!instance->has_patients)

include/pybind11/pybind11.h

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1087,9 +1087,15 @@ class class_ : public detail::generic_type {
10871087
static void holder_reclaim(detail::value_and_holder& v_h, void* external_holder_raw) {
10881088
// Reclaim from `external_holder` into `v_h.holder<...>()`.
10891089
assert(!v_h.inst->owned && !v_h.holder_constructed() && "Internal error: Object must not be owned");
1090-
assert(external_holder_raw && "Internal error: External holder must not be null");
1091-
holder_type& external_holder = *reinterpret_cast<holder_type*>(external_holder_raw);
1092-
new (&v_h.holder<holder_type>()) holder_type(std::move(external_holder));
1090+
holder_type& holder = v_h.holder<holder_type>();
1091+
if (external_holder_raw) {
1092+
// Take from external holder.
1093+
holder_type& external_holder = *reinterpret_cast<holder_type*>(external_holder_raw);
1094+
new (&holder) holder_type(std::move(external_holder));
1095+
} else {
1096+
// Construct new holder, using existing value.
1097+
new (&holder) holder_type(v_h.value_ptr<type>());
1098+
}
10931099
v_h.set_holder_constructed();
10941100
v_h.inst->owned = true;
10951101
// If this instance is now owend by pybind, release any existing

tests/test_smart_ptr.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,8 +89,7 @@ class Container {
8989
} else {
9090
cls.def(py::init<Ptr>());
9191
}
92-
// TODO: Figure out why reference_internal does not work???
93-
cls.def("get", &Container::get, py::keep_alive<0, 1>()); //py::return_value_policy::reference_internal);
92+
cls.def("get", &Container::get, py::return_value_policy::reference_internal);
9493
cls.def("release", &Container::release);
9594
cls.def("reset", &Container::reset);
9695
}

0 commit comments

Comments
 (0)