-
-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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-105340: include hidden fast-locals in locals() #105715
Merged
Merged
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
5609f64
gh-105340: include hidden fast-locals in locals()
carljm 2fae889
Merge branch 'main' into modulelocals
carljm bd0c357
fixes
carljm 166aca9
Merge branch 'main' into modulelocals
carljm b096651
Merge branch 'main' into modulelocals
carljm 1e27721
also fix dir()
carljm ac11740
Merge branch 'main' into modulelocals
carljm ab95d4b
rename PyEval_GetFrameLocals to _PyEval_GetLocals and make it private
carljm 7c33113
don't use PyAPI_FUNC for _PyEval_GetLocals
carljm 4c59e51
Merge branch 'main' into modulelocals
carljm 6ec94bf
Rename to _PyEval_GetFrameLocals, so its less similar to PyEval_GetLo…
carljm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 2 additions & 0 deletions
2
Misc/NEWS.d/next/Core and Builtins/2023-06-12-16-38-31.gh-issue-105340._jRHXe.rst
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Include the comprehension iteration variable in ``locals()`` inside a | ||
module- or class-scope comprehension. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Does this still need to be a PyAPI_FUNC? Since it's only used in the core, seems like we could leave it as simply
extern
like the function immediately above.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.
I wasn't sure what was preferred here; there are also lots of examples of functions declared in
pycore_*.h
files (including this header) that usePyAPI_FUNC
. But the definition ofPyAPI_FUNC
says it is for "public API functions," and I think you're right that it's not needed, so I'll remove it.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.
https://devguide.python.org/developer-workflow/c-api/#with-the-extern-keyword says to use extern when in doubt
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.
I don't think
extern
does anything here, but some people prefer it for style reasons, and it seems like most (not all) non-static function declarations in this file use it, so I'll use it.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.
If the function is to be used by extension modules that are part of the standard library (or say they are, by defining Py_BUILD_CORE), which can be built as .so files, you need to use PyAPI_FUNC. Otherwise the symbol may not be exported on Windows (and I believe a few other platforms, like Android) or when using
-fvisibility=hidden
.FWIW, I hadn't realised you're adding a
_PyEval_GetLocals()
when there's already aPyEval_GetLocals()
that does a subtly different thing. I'm not fond of using the same name with just a_
difference here.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.
I could make it
_PyEval_GetLocalsRef
(c.f. python/steering-council#201) since the one notable difference is that it returns a new reference.The other notable difference is the behavior inside an inlined comprehension in module or class scope, but I don't think there is any way to summarize that difference in a name.
This function will go away in 3.13 if we do PEP 667, or something like it, to standardize the behavior of
locals()
.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.
Currently there's no use of the function in any standard library extension modules (
PyEval_GetLocals
was used only once inobject.c
and several times inbltinmodule.c
), and I don't expect it to be needed in any, so I don't think we need to usePyAPI_FUNC
.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.
Or, I could go back to using
_PyEval_GetFrameLocals
like I did originally in this PR (but this time with a leading underscore). If we get PEP 667, it would introducePyEval_GetFrameLocals
as public API, and the private_PyEval_GetFrameLocals
wouldn't be needed anymore and could be removed.