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.

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

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

picnixz
Copy link
Contributor

@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
Modules/_cursesmodule.c Outdated Show resolved Hide resolved
Doc/whatsnew/3.14.rst Outdated Show resolved Hide resolved
Modules/_cursesmodule.c Outdated Show resolved Hide resolved
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
Contributor 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. 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants