Uh oh!
There was an error while loading. Please reload this page.
gh-103509: PEP 697 -- Limited C API for Extending Opaque Types - #103511
Conversation
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.
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.
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! |
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.
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.
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