gh-128563: Move lltrace into the frame struct#129113
gh-128563: Move lltrace into the frame struct#129113Fidget-Spinner merged 15 commits intopython:mainfrom
Conversation
This reverts commit 6ba4e59.
markshannon
left a comment
There was a problem hiding this comment.
The change to the size of the frame can be avoided by using a bitfield.
I'm curious why the atomic loads and stores are needed.
|
When you're done making the requested changes, leave the comment: |
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @markshannon: please review the changes made to this pull request. |
Include/internal/pycore_frame.h
Outdated
| char owner; | ||
| #ifdef Py_DEBUG | ||
| char visited:4; | ||
| char lltrace:4; |
There was a problem hiding this comment.
Why 4?
You only need one bit (although it is best to use uint8_t as the signedness of char is implementation defined.
My mistake, lltrace is not boolean.
Since char might be signed, it is best to make this a uint8_t to avoid overflow for PYTHON_LLTRACE=9
markshannon
left a comment
There was a problem hiding this comment.
One small issue, otherwise looks good.
lltrace should be unsigned (uint8_t), and maybe bigger, to avoid overflow. visited only needs one bit, so lltrace could use the rest.
For consistency, make visited a uint8_t in the non-debug build as well.
|
Sorry for being late here. These lines seem to be unused, since the PR did not have to touch them? Lines 1066 to 1071 in 05d12ee |
|
@chris-eibl it seems so. If you would like to, could you please open a PR to either update to fix them or remove them? Thanks! |
|
Yeah, but since this is gonna be my first PR, I'll have to work myself through the devguide. Most probably not before the weekend ... |
Uh oh!
There was an error while loading. Please reload this page.