Skip to content
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

Added region check to tuple object #46

Merged
merged 1 commit into from
Feb 24, 2025
Merged

Conversation

TobiasWrigstad
Copy link
Collaborator

As far as I can see in the source code, tuples are always created in the local region (effectively), and get their arguments before they are assigned anywhere. So the only check added to set_item should always succeed. Leaving optimising this case for future work.

As far as I can see in the source code, tuples are always created
in the local region (effectively), and get their arguments before
they are assigned anywhere. So the only check added to set_item
should always succeed. Leaving optimising this case for future work.
@mjp41
Copy link
Owner

mjp41 commented Feb 2, 2025

Just read
https://stackoverflow.com/questions/6111843/limitations-of-pytuple-setitem

Which might suggest we can't always optimise. Also not sure what we should do if a C module calls the macro PyTuple_SETITEM

@TobiasWrigstad
Copy link
Collaborator Author

Thanks for finding that. So let's keep this as is!

@xFrednet
Copy link
Collaborator

xFrednet commented Feb 4, 2025

The changes look good to me.

Which might suggest we can't always optimise. Also not sure what we should do if a C module calls the macro PyTuple_SETITEM

I'd say it depends on the module the macro is defined in. Since this one appears to be public (No pycore_ prefix), I feel like we need to add a write check in some shape or form.

On the other hand, macro/function doesn't do a bounds check and just allows indexing anywhere.

We could mark the region as "dirty" when an item is set via this function. But this might be too strict in some cased. So I'd suggest adding a TODO: Pyrona: above the macro and checking that all uses in cpython have a proper write guard.

@TobiasWrigstad
Copy link
Collaborator Author

@mjp41 Can I merge this?

@TobiasWrigstad
Copy link
Collaborator Author

Merging after discussion with @mjp41

@TobiasWrigstad TobiasWrigstad merged commit 349a598 into phase3 Feb 24, 2025
8 of 9 checks passed
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.

3 participants