Skip to content

Properly visit self in >=3.9 traverse#4051

Merged
rwgk merged 2 commits into
pybind:masterfrom
Skylion007:skylion007/39-traverse-self
Jul 10, 2022
Merged

Properly visit self in >=3.9 traverse#4051
rwgk merged 2 commits into
pybind:masterfrom
Skylion007:skylion007/39-traverse-self

Conversation

@Skylion007

Copy link
Copy Markdown
Collaborator

Description

  • This PR fixes a small bug that was already fixed in nanobind. Specifically, the our traverse behavior does not follow the clarified behavior of the tp-traverse function Python >=3.9.
  • I originally discovered this bug when researching the 3.11 fixes, but this did not fix the root cause there. Still, it's better to have proper behavior for >= 3.9 so this PR cherry picks that fix and ensure we properly traverse the classes when calling gc on dynamically allocated classes.

Suggested changelog entry:

* Ensure proper behavior when garbage collecting classes with dynamic attributes in Python >=3.9.

@Skylion007
Skylion007 marked this pull request as ready for review July 8, 2022 16:03
@Skylion007
Skylion007 requested review from henryiii and rwgk July 8, 2022 16:03
PyObject *&dict = *_PyObject_GetDictPtr(self);
Py_VISIT(dict);
#if PY_VERSION_HEX >= 0x03090000
Py_VISIT(Py_TYPE(self));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: I'd add this comment (your link from the PR description) so the next person to look here has an easier time understanding this code:

// https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_traverse

@rwgk

rwgk commented Jul 10, 2022

Copy link
Copy Markdown
Collaborator

Thanks @Skylion007, I'll go ahead merging this PR, to get it into the next smart_holder update.

@rwgk
rwgk merged commit f9f0049 into pybind:master Jul 10, 2022
@github-actions github-actions Bot added the needs changelog Possibly needs a changelog entry label Jul 10, 2022
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants