-
-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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-118934: Make PyEval_GetLocals return borrowed reference #119769
Conversation
Just realized this will overwrite |
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.
A few formatting suggestions and a question about the return value
@gaogaotiantian I merged the suggestion that reverts back to the Python 3.12 semantics, so we can get a CI run on that version of the refleak fix. |
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.
@gaogaotiantian CI is happy with going back to the Python 3.12 behaviour, so if you're happy I think this is good to merge for the final beta.
Thanks @gaogaotiantian for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13. |
…honGH-119769) (cherry picked from commit e65cb4c) Co-authored-by: Tian Gao <[email protected]> Co-authored-by: Alyssa Coghlan <[email protected]>
GH-121869 is a backport of this pull request to the 3.13 branch. |
…-119769) (#121869) gh-118934: Make PyEval_GetLocals return borrowed reference (GH-119769) (cherry picked from commit e65cb4c) Co-authored-by: Tian Gao <[email protected]> Co-authored-by: Alyssa Coghlan <[email protected]>
|
|
Per discussion with @markshannon in Pittsburgh, we simulate what we did pre-PEP667. We store the locals dict in
frame->f_locals
so that we can return a borrowed reference. Theframe->f_locals
will be cleared when the frame is released. This way we can keep the behavior ofPyEval_GetLocals()
.PyEval_GetLocals()
leaks locals #118934