Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bpo-44301: Docs: Note that tp_clear and m_clear are not always called #27581

Merged
merged 1 commit into from
Aug 4, 2021
Merged

bpo-44301: Docs: Note that tp_clear and m_clear are not always called #27581

merged 1 commit into from
Aug 4, 2021

Conversation

encukou
Copy link
Member

@encukou encukou commented Aug 3, 2021 •
edited by bedevere-bot
Loading

It was not clear from the documentation that tp_clear (or in this case, m_clear of modules) is only called by the cyclic GC; when the object reaches zero refcount, tp_clear isn't called.

https://bugs.python.org/issue44301

@encukou encukou added docs Documentation in the Doc dir skip news needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Aug 3, 2021
@encukou encukou requested review from vstinner and pablogsal August 3, 2021 15:37
@encukou
Copy link
Member Author

encukou commented Aug 3, 2021

I still intend to write some better tutorial-style docs on supporting GC. For the reference docs, I think this quick note is useful enough.

pablogsal
pablogsal approved these changes Aug 3, 2021
View reviewed changes
Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

LGTM

A common pattern thought is to invoke tp_clear in tp_dealloc

@encukou
Copy link
Member Author

encukou commented Aug 3, 2021

A common pattern thought is to invoke tp_clear in tp_dealloc

That's mentioned in the paragraph below (for types; for modules there's a link to PyTypeObject.tp_clear).
I'll cover best practices in another PR, but it looks like that'll include a bigger refactoring.

@ambv ambv merged commit 10faada into python:main Aug 4, 2021
@miss-islington
Copy link
Contributor

Thanks @encukou for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.9.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 4, 2021
@encukou @miss-islington
Note that tp_clear and m_clear are not always called (pythonGH-27581)
9ef95c7 
(cherry picked from commit 10faada)

Co-authored-by: Petr Viktorin <encukou@gmail.com>
@bedevere-bot
Copy link

GH-27596 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Aug 4, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 4, 2021
@encukou @miss-islington
Note that tp_clear and m_clear are not always called (pythonGH-27581)
16ac231 
(cherry picked from commit 10faada)

Co-authored-by: Petr Viktorin <encukou@gmail.com>
@bedevere-bot
Copy link

GH-27597 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Aug 4, 2021
miss-islington added a commit that referenced this pull request Aug 4, 2021
@miss-islington @encukou
Note that tp_clear and m_clear are not always called (GH-27581)
f4b3217 
(cherry picked from commit 10faada)

Co-authored-by: Petr Viktorin <encukou@gmail.com>
ambv pushed a commit that referenced this pull request Aug 4, 2021
@miss-islington @encukou
Note that tp_clear and m_clear are not always called (GH-27581) (GH-2…
467c873 
…7597)

(cherry picked from commit 10faada)

Co-authored-by: Petr Viktorin <encukou@gmail.com>
@encukou encukou deleted the bpo-44301 branch August 5, 2021 07:52
@vstinner
Copy link
Member

vstinner commented Aug 6, 2021

Thanks @encukou, implementing a type in C is complex, and better documentation is very valuable!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablogsal pablogsal pablogsal approved these changes

@vstinner vstinner Awaiting requested review from vstinner

Assignees
No one assigned
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@encukou @miss-islington @bedevere-bot @vstinner @pablogsal @ambv @the-knights-who-say-ni