Skip to content

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

Merged
vstinner merged 9 commits into
python:mainfrom
picnixz:curses/heap-type-123961
Oct 4, 2024
Merged

gh-123961: convert curses.window static type into a heap type#124934
vstinner merged 9 commits into
python:mainfrom
picnixz:curses/heap-type-123961

Conversation

@picnixz

@picnixzpicnixz commented Oct 3, 2024

Copy link
Copy Markdown
Member

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 vstinnerOctober 3, 2024 12:58
@Eclips4
Eclips4 self-requested a review October 3, 2024 14:00
Comment threadModules/_cursesmodule.c Outdated
Comment threadDoc/whatsnew/3.14.rst Outdated
Comment threadModules/_cursesmodule.c Outdated

@vstinnervstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@picnixz

Copy link
Copy Markdown
MemberAuthor

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

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

Copy link
Copy Markdown

🤖 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-botbedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Oct 3, 2024
@vstinner

Copy link
Copy Markdown
Member

There is no ref leak:

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

Comment threadMisc/NEWS.d/next/C_API/2024-10-03-14-06-08.gh-issue-123961.uwJQTY.rst Outdated
We also remove the use of a the `file` role since we incorrectly used it :')

@Eclips4Eclips4 left a comment

Copy link
Copy Markdown
Member

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:mainOct 4, 2024
@vstinner

Copy link
Copy Markdown
Member

Merged, thanks.

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

@picnixz@bedevere-bot@vstinner@Eclips4