Skip to content

GH-111537: Avoid using this_instr in asserts. #111600

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

Merged

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Nov 1, 2023

Replace frame->return_offset = 1 + INLINE_CACHE_ with frame->return_offset = next_instr - this_instr
As we now specify this_instr in terms of next_instr, we can rely on the C compiler to replace next_instr - this_instr with a constant

@@ -657,8 +657,7 @@ dummy_func(
STACK_SHRINK(2);
new_frame->localsplus[0] = container;
new_frame->localsplus[1] = sub;
assert(1 + INLINE_CACHE_ENTRIES_BINARY_SUBSCR == next_instr - this_instr);
frame->return_offset = 1 + INLINE_CACHE_ENTRIES_BINARY_SUBSCR;
frame->return_offset = next_instr - this_instr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably needs a cast to uint16_t

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's guaranteed to be small. So, I think it should be OK. Let's see if MSVC complains.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, indeed, complain

@markshannon markshannon merged commit 5697fc2 into python:main Nov 1, 2023
FullteaR pushed a commit to FullteaR/cpython that referenced this pull request Nov 3, 2023
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@markshannon markshannon deleted the dont-use-cache-entries-sizes branch August 6, 2024 10:17
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants