The Wayback Machine - https://web.archive.org/web/20250512071935/https://github.com/python/cpython/issues/133713
Skip to content

Compare the f->stackpointer to the result of _PyFrame_Stackbase(f) #133713

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.

Sign up for GitHub

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

Jump to bottom
Closed
nybblista opened this issue May 8, 2025 · 2 comments
Closed

Compare the f->stackpointer to the result of _PyFrame_Stackbase(f) #133713

nybblista opened this issue May 8, 2025 · 2 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@nybblista
Copy link
Contributor

nybblista commented May 8, 2025 •
edited by bedevere-app bot
Loading

I think we should compare f->stackpointer to the result of the _PyFrame_Stackbase(f) in the _PyFrame_StackPeek and in the _PyFrame_StackPop rather than repeat the offset calculation in both of them.

cpython/Include/internal/pycore_interpframe.h

Lines 46 to 60 in c492ac7

static inline _PyStackRef *_PyFrame_Stackbase(_PyInterpreterFrame *f) {
return (f->localsplus + _PyFrame_GetCode(f)->co_nlocalsplus);
}
static inline _PyStackRef _PyFrame_StackPeek(_PyInterpreterFrame *f) {
assert(f->stackpointer > f->localsplus + _PyFrame_GetCode(f)->co_nlocalsplus);
assert(!PyStackRef_IsNull(f->stackpointer[-1]));
return f->stackpointer[-1];
}
static inline _PyStackRef _PyFrame_StackPop(_PyInterpreterFrame *f) {
assert(f->stackpointer > f->localsplus + _PyFrame_GetCode(f)->co_nlocalsplus);
f->stackpointer--;
return *f->stackpointer;
}

Linked PRs
@bedevere-app bedevere-app bot mentioned this issue May 8, 2025
Merged
@picnixz
Copy link
Member

picnixz commented May 8, 2025 •
edited
Loading

Those are assertions. We don't really need to be efficient in DEUBG builds so I don't think it's worth the change. OTOH it could be a cleanup. Up to @markshannon then

@picnixz picnixz added type-feature A feature request or enhancement interpreter-core (Objects, Python, Grammar, and Parser dirs) pending The issue will be closed if no feedback is provided labels May 8, 2025
@ZeroIntensity
Copy link
Member

Moreover: I'd like to avoid these kinds of issues/PRs in the future. This sort of thing should happen in a PR review, but once the change has already landed, it's just needless refactoring.

Fidget-Spinner pushed a commit that referenced this issue May 10, 2025
@nybblista
gh-133713: Compare the f->stackpointer to the result of `_PyFrame_S…
8320582 
…tackbase(f)` (GH-133714)
@ZeroIntensity ZeroIntensity removed the pending The issue will be closed if no feedback is provided label May 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@picnixz @Fidget-Spinner @ZeroIntensity @nybblista