Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-134584: Cleanups for refcount elimination #135860#142604
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Fidget-Spinner commented Dec 11, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
corona10 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
markshannon left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make a separate PRs for fixing the leak? I'd like to get that in ASAP.
For the larger changes:
- There is no need for
_POP_TOP_NOT_NULLas it still needs to check for immortal objects andNULLis immortal - The changes to the code generator need more explanation and maybe a new issue.
Uh oh!
There was an error while loading. Please reload this page.
When you're done making the requested changes, leave the comment: |
Fidget-Spinner commented Dec 12, 2025
I can't without a partial revert. The problem is that we don't allow live inputs at ERROR_IF. This PR simply changes the ERROR_IF behavior to close all live inputs. The rest of this PR to the cases generator is just moving stuff around if you take a closer look. I mainly had to expose the close_variable function as it was previously a nested one. |
Zheaoli commented Dec 12, 2025
Just for personal thought(correct me plz if I'm wrong) The root cause about this refleak is related with the wrong position about So the core point about this PR is the behavior change "raise an exception when here's live variable on TOS -> close all the live variable" So we can mark the stack |
Fidget-Spinner commented Dec 12, 2025
@Zheaoli yes you're right |
Fidget-Spinner commented Dec 13, 2025
I removed all the changes to the code generator and POP_TOP_NOT_NULL. As such, this review is now stale. |
I removed all the changes that were contentious. This PR is now just a re-implementation of #135860. So it's fine now as that was previously approved and merged.
corona10 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the current change is only a reimplementation of _CALL_TUPLE_1 from #135860, it looks good to me.
e02a35c into python:mainUh oh!
There was an error while loading. Please reload this page.
bedevere-bot commented Dec 13, 2025
|
This cleans up PR #135860.
Mostly just moving code around.