Skip to content

gh-123961: convert curses.window static type into a heap type #124934

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.

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

Merged
merged 9 commits into from
Oct 4, 2024

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Oct 3, 2024

I think this should do the job. I observed that there are more reference leaks but I think it's normal since I don't know of any way to actually clear the heap type in a single-phase initialization module.

The only way I can think of clearing it is to add some finalization callback at interpreter's shutdown but I think this is not a good idea because we'll likely transform the module into a multi-phase initialization module in the next PR.

However, I'd like to have some additional eyes (cc @encukou @Eclips4) to indeed check that the reference leaks that I introduced are inevitable and not due to how I defined the heap type.


📚 Documentation preview 📚: https://cpython-previews--124934.org.readthedocs.build/

@picnixz picnixz requested a review from vstinner October 3, 2024 12:58
@Eclips4 Eclips4 self-requested a review October 3, 2024 14:00
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

@picnixz
Copy link
Member Author

picnixz commented Oct 3, 2024

Since I'm waiting for Kirill's review, I'll also run the build bots!

@picnixz picnixz added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Oct 3, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @picnixz for commit 9c787bc 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Oct 3, 2024
@vstinner
Copy link
Member

vstinner commented Oct 3, 2024

There is no ref leak:

$ ./python -m test test_curses -u all -R 3:3
(...)
Result: SUCCESS

We also remove the use of a the `file` role since we incorrectly used it :')
Copy link
Member

@Eclips4 Eclips4 left a comment

Choose a reason for hiding this comment

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

LGTM. 🚀

@vstinner vstinner merged commit f66d785 into python:main Oct 4, 2024
37 checks passed
@vstinner
Copy link
Member

vstinner commented Oct 4, 2024

Merged, thanks.

@picnixz picnixz deleted the curses/heap-type-123961 branch October 4, 2024 10:10
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.

4 participants