bpo-45256: Don't track the exact depth of each InterpreterFrame#30372
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Include/internal/pycore_frame.h Outdated
| intstacktop; /* Offset of TOS from localsplus */ | ||
| PyFrameStatef_state; /* What state the frame is in */ | ||
| intdepth; /* Depth of the frame in a ceval loop */ | ||
| boolown_cframe; // Whether this is the "root" frame for the current CFrame |
There was a problem hiding this comment.
I'm not sure about this name.
I don't think the frame really owns the C frame. Also, be wary of using CFrame, as CFrame is a struct name.
If I were to suggest a name, it would be is_entry as this is the entry InterpreterFrame for _PyEval_EvalFrameDefault.
If you think ownership is a better mental model, then owns_cframe is fine.
There was a problem hiding this comment.
@markshannon Can it be named backed_by_cframe or cframe_backed instead?
There was a problem hiding this comment.
This has nothing to do with how the data is stored.
There was a problem hiding this comment.
I agree with Mark, I think it should be called entry_frame or something similar. Reasoning about ownership is a bit more obscure in my opinion
There was a problem hiding this comment.
I was hoping somebody would suggest a better name! is_entry it is...
Include/internal/pycore_frame.h Outdated
| frame->f_lasti=-1; | ||
| frame->f_state=FRAME_CREATED; | ||
| frame->depth=0; | ||
| frame->own_cframe=true; |
There was a problem hiding this comment.
This should default to false. Then it only need be set at the start of _PyEval_EvalFrameDefault, not for every internal call.
markshannon left a comment
markshannon left a comment There was a problem hiding this comment.
A couple of minor issues, but sound in general.
bedevere-bot commented Jan 4, 2022
When you're done making the requested changes, leave the comment: |
6 participants
We only really care whether or not an exiting frame is the "root" of the current
CFrame, which a boolean value can track more efficiently.(In fact, the current code is buggy, and doesn't even maintain
frame->depthcorrectly... it's only ever set to0or1anyways!)https://bugs.python.org/issue45256