Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-103509: PEP 697 -- Limited C API for Extending Opaque Types#103511
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
encukou commented Apr 13, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
encukou commented Apr 17, 2023
What am I missing? (The reference I linked to lists |
zooba commented Apr 17, 2023
Looks like you need |
encukou commented Apr 19, 2023
That means I can't use |
bedevere-bot commented Apr 20, 2023
arhadthedev commented Apr 20, 2023
The TraceRefs buildbot failure:
is likely caused by gh-103621. |
arhadthedev left a comment
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bedevere-bot commented Apr 27, 2023
Smaller objects might not be aligned to ALIGNOF_MAX_ALIGN_T. The offsets calculated for PEP 697 should be aligned, though.
bedevere-bot commented Apr 28, 2023
erlend-aasland left a comment
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
encukou commented May 2, 2023
Thank you for the thorough review! |
erlend-aasland commented May 2, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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.) |
arhadthedev left a comment
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
encukou left a comment
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 :)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
encukou commented May 4, 2023
Thanks for the reviews! I'll merge, and leave the issues open for other PRs. |
erlend-aasland commented May 4, 2023
Good job! This is a very nice feature. |
encukou commented May 4, 2023
Thank you! FWIW, here's a follow-up on the |
PEP 697 describes the change. Some details it doesn't cover:
_PyHeapType_GET_MEMBERSis moved fromInclude/internalto the only.cfile that uses it.TODO:
alignof&max_align_t