-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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-103509: PEP 697 -- Limited C API for Extending Opaque Types #103511
Conversation
What am I missing? (The reference I linked to lists |
Looks like you need |
That means I can't use |
…e flags Now is not the time to fix that.
The TraceRefs buildbot failure:
is likely caused by gh-103621. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void*
cannot participate in address arithmetics. You need to cast both data_ptr
and instance
to char *
to get distance in bytes.
Smaller objects might not be aligned to ALIGNOF_MAX_ALIGN_T. The offsets calculated for PEP 697 should be aligned, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
Sorry for the late review; it took some time to dig through everything.
Co-authored-by: Erlend E. Aasland <[email protected]>
Thank you for the thorough review! |
Thanks! I've got some more comments, but we can deal with those in a follow-up PR if you think it's worth it. Let's land this before the feature freeze kicks in. (I'm on mobile now; I won't be able to check the changes until tomorrow.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, keep in mind that I neither participated in the PR discussion nor seen the PEP before. So I looked at the rendered docs in the same fashion as an outside user would do.
Here are three optional rough proposals and a forgotten full stop. However, they are non-binding suggestions that can be properly considered later, after 3.12b1 is out (otherwise the two remaining weeks will pass swiftly in the bikeshedding).
I've limited myself to the documentation because I have no expertise in the C API to check such a volume of code in such a short period of time.
Co-authored-by: Oleg Iarygin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Fresh eyes is exactly what the documentation needs.
It will also need a tutorial to bring the concepts together -- but when trying to write a tutorial, I keep finding rough edges that should be fixed first :)
Thanks for the reviews! I'll merge, and leave the issues open for other PRs. |
Good job! This is a very nice feature. |
Thank you! FWIW, here's a follow-up on the |
PEP 697 describes the change. Some details it doesn't cover:
_PyHeapType_GET_MEMBERS
is moved fromInclude/internal
to the only.c
file that uses it.TODO:
alignof
&max_align_t