Skip to content

Conversation

@Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Dec 2, 2025

I verified this fixes the repro given in the issue. I'm not adding the repro because it's not self-contained, and so might change if asyncio changes.

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

Why the extra code in optimizer_genrator.py when we're just changing assert(WITHIN_STACK_BOUNDS()) to CHECK_STACK_BOUNDS()?

You can reduce the amount of extra code by making the assertion/check code an attribute of the Stack object.

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

One piece of code left from the earlier version that should be removed.
Otherwise, LGTM.

def emit_save(self, storage: Storage) -> None:
storage.flush(self.out)

def sync_sp(
Copy link
Member

Choose a reason for hiding this comment

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

This is now just a copy of the overridden method, so it can be removed.

@bedevere-app
Copy link

bedevere-app bot commented Dec 4, 2025

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@Fidget-Spinner
Copy link
Member Author

Actually, there's a serious flaw with this approach.

It should be checking the stack bound before we write and increment the stack pointer. Otherwise, we might be writing to invalid memory.

@Fidget-Spinner
Copy link
Member Author

I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented Dec 4, 2025

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from markshannon December 4, 2025 13:32
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

LGTM.

The extra checks probably aren't strictly necessary for safety, since we are writing into the middle of a huge buffer.
But, they're not wrong and a bit of extra safety is a good thing.

@Fidget-Spinner Fidget-Spinner merged commit b3bf212 into python:main Dec 4, 2025
75 of 76 checks passed
@Fidget-Spinner Fidget-Spinner deleted the optimizer_check branch December 4, 2025 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants