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

gh-123465: Allow Py_RELATIVE_OFFSET for __*offset__ members #123474

Merged
merged 3 commits into from
Sep 5, 2024

Conversation

encukou
Copy link
Member

@encukou encukou commented Aug 29, 2024

Allow flags = Py_READONLY | Py_RELATIVE_OFFSET for the special offset members,
which previously required Py_READONLY.


📚 Documentation preview 📚: https://cpython-previews--123474.org.readthedocs.build/

@encukou encukou marked this pull request as ready for review August 30, 2024 05:54
@wjakob
Copy link
Contributor

wjakob commented Sep 3, 2024

Thank you, I confirm that this PR fixes the issue I encountered.

@encukou encukou merged commit 16be8db into python:main Sep 5, 2024
36 of 37 checks passed
@encukou encukou deleted the special_relative_offset branch September 5, 2024 12:14
Comment on lines +4740 to +4748
if (strcmp(memb->name, "__weaklistoffset__") == 0) {
weaklistoffset_member = memb;
}
if (strcmp(memb->name, "__dictoffset__") == 0) {
dictoffset_member = memb;
}
if (strcmp(memb->name, "__vectorcalloffset__") == 0) {
vectorcalloffset_member = memb;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to use if-else or continue. Non-debug builds on MSVC currently avoid a crash here by relying on a default SSA-optimization option (partial redundancy elimination) for some reason, which is usually disabled (/d2ssa-pre-) to diagnose or work around a crash unlike this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding else sounds like a great change! continue would work too.
You could reuse gh-123465 to keep the fix-up commit grouped with this one.

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