Skip to content

Commit

Permalink
Address pull review feedback from @jagerman
Browse files Browse the repository at this point in the history
- Clear has_patients in clear_instance
- Handle multiple inheritence
- Use ConstructorStats.detail_reg_inst to do deep garbage collection and
  verify the instance tracking counts
  • Loading branch information
bmerry committed Jun 13, 2017
1 parent d225c7f commit f93156c
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 22 deletions.
1 change: 1 addition & 0 deletions include/pybind11/class_support.h
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,7 @@ inline void clear_instance(PyObject *self) {
// from the unordered_map first.
auto patients = std::move(pos->second);
internals.patients.erase(pos);
instance->has_patients = false;
for (PyObject *&patient : patients)
Py_CLEAR(patient);
}
Expand Down
4 changes: 2 additions & 2 deletions include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -1334,8 +1334,8 @@ inline void keep_alive_impl(handle nurse, handle patient) {
if (patient.is_none() || nurse.is_none())
return; /* Nothing to keep alive or nothing to be kept alive by */

auto tinfo = get_type_info(Py_TYPE(nurse.ptr()));
if (tinfo) {
auto tinfo = all_type_info(Py_TYPE(nurse.ptr()));
if (!tinfo.empty()) {
/* It's a pybind-registered type, so we can store the patient in the
* internal list. */
add_patient(nurse.ptr(), patient.ptr());
Expand Down
68 changes: 48 additions & 20 deletions tests/test_call_policies.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,67 +2,69 @@


def test_keep_alive_argument(capture):
from pybind11_tests import Parent, Child
from pybind11_tests import Parent, Child, ConstructorStats

n_inst = ConstructorStats.detail_reg_inst()
with capture:
p = Parent()
assert capture == "Allocating parent."
with capture:
p.addChild(Child())
pytest.gc_collect()
assert ConstructorStats.detail_reg_inst() == n_inst + 1
assert capture == """
Allocating child.
Releasing child.
"""
with capture:
del p
pytest.gc_collect()
assert ConstructorStats.detail_reg_inst() == n_inst
assert capture == "Releasing parent."

with capture:
p = Parent()
assert capture == "Allocating parent."
with capture:
p.addChildKeepAlive(Child())
pytest.gc_collect()
assert ConstructorStats.detail_reg_inst() == n_inst + 2
assert capture == "Allocating child."
with capture:
del p
pytest.gc_collect()
assert ConstructorStats.detail_reg_inst() == n_inst
assert capture == """
Releasing parent.
Releasing child.
"""


def test_keep_alive_return_value(capture):
from pybind11_tests import Parent
from pybind11_tests import Parent, ConstructorStats

n_inst = ConstructorStats.detail_reg_inst()
with capture:
p = Parent()
assert capture == "Allocating parent."
with capture:
p.returnChild()
pytest.gc_collect()
assert ConstructorStats.detail_reg_inst() == n_inst + 1
assert capture == """
Allocating child.
Releasing child.
"""
with capture:
del p
pytest.gc_collect()
assert ConstructorStats.detail_reg_inst() == n_inst
assert capture == "Releasing parent."

with capture:
p = Parent()
assert capture == "Allocating parent."
with capture:
p.returnChildKeepAlive()
pytest.gc_collect()
assert ConstructorStats.detail_reg_inst() == n_inst + 2
assert capture == "Allocating child."
with capture:
del p
pytest.gc_collect()
assert ConstructorStats.detail_reg_inst() == n_inst
assert capture == """
Releasing parent.
Releasing child.
Expand All @@ -72,66 +74,92 @@ def test_keep_alive_return_value(capture):
# https://bitbucket.org/pypy/pypy/issues/2447
@pytest.unsupported_on_pypy
def test_alive_gc(capture):
from pybind11_tests import ParentGC, Child
from pybind11_tests import ParentGC, Child, ConstructorStats

n_inst = ConstructorStats.detail_reg_inst()
p = ParentGC()
p.addChildKeepAlive(Child())
assert ConstructorStats.detail_reg_inst() == n_inst + 2
lst = [p]
lst.append(lst) # creates a circular reference
with capture:
del p, lst
pytest.gc_collect()
assert ConstructorStats.detail_reg_inst() == n_inst
assert capture == """
Releasing parent.
Releasing child.
"""


def test_alive_gc_derived(capture):
from pybind11_tests import Parent, Child
from pybind11_tests import Parent, Child, ConstructorStats

class Derived(Parent):
pass

n_inst = ConstructorStats.detail_reg_inst()
p = Derived()
p.addChildKeepAlive(Child())
assert ConstructorStats.detail_reg_inst() == n_inst + 2
lst = [p]
lst.append(lst) # creates a circular reference
with capture:
del p, lst
pytest.gc_collect()
pytest.gc_collect() # Needed to make PyPy free the child
assert ConstructorStats.detail_reg_inst() == n_inst
assert capture == """
Releasing parent.
Releasing child.
"""


def test_alive_gc_multi_derived(capture):
from pybind11_tests import Parent, Child, ConstructorStats

class Derived(Parent, Child):
pass

n_inst = ConstructorStats.detail_reg_inst()
p = Derived()
p.addChildKeepAlive(Child())
# +3 rather than +2 because Derived corresponds to two registered instances
assert ConstructorStats.detail_reg_inst() == n_inst + 3
lst = [p]
lst.append(lst) # creates a circular reference
with capture:
del p, lst
assert ConstructorStats.detail_reg_inst() == n_inst
assert capture == """
Releasing parent.
Releasing child.
"""


def test_return_none(capture):
from pybind11_tests import Parent
from pybind11_tests import Parent, ConstructorStats

n_inst = ConstructorStats.detail_reg_inst()
with capture:
p = Parent()
assert capture == "Allocating parent."
with capture:
p.returnNullChildKeepAliveChild()
pytest.gc_collect()
assert ConstructorStats.detail_reg_inst() == n_inst + 1
assert capture == ""
with capture:
del p
pytest.gc_collect()
assert ConstructorStats.detail_reg_inst() == n_inst
assert capture == "Releasing parent."

with capture:
p = Parent()
assert capture == "Allocating parent."
with capture:
p.returnNullChildKeepAliveParent()
pytest.gc_collect()
assert ConstructorStats.detail_reg_inst() == n_inst + 1
assert capture == ""
with capture:
del p
pytest.gc_collect()
assert ConstructorStats.detail_reg_inst() == n_inst
assert capture == "Releasing parent."


Expand Down

0 comments on commit f93156c

Please sign in to comment.