gh-103509: PEP 697 -- Limited C API for Extending Opaque Types by encukou · Pull Request #103511 · python/cpython · GitHub

PEP 697 describes the change. Some details it doesn't cover:

_PyHeapType_GET_MEMBERS is moved from Include/internal to the only .c file that uses it.
Tests demonstrate the API is usable for the purpose, and verify the layout with brutish-force.

TODO:

What's New entry
Fix the alignof & max_align_t

Issue: PEP 697 -- Limited C API for Extending Opaque Types #103509

typeobject.c includes <stddef.h> and <stdalign.h>, which should define max_align_t and alignof in C11. Yet, the Windows build complains:

D:\a\cpython\cpython\Objects\typeobject.c(3558,34): warning C4013: 'alignof' undefined; assuming extern returning int [D:\a\cpython\cpython\PCbuild\_freeze_module.vcxproj]
D:\a\cpython\cpython\Objects\typeobject.c(3558,53): error C2065: 'max_align_t': undeclared identifier [D:\a\cpython\cpython\PCbuild\_freeze_module.vcxproj]

What am I missing?
@zooba, could you help?

(The reference I linked to lists alignof as removed in C23, which is a bit misleading. The macro will be removed because it will become a keyword.)

Looks like you need stdalign.h for alignof (defines the alignof macro to _Alignof), and max_align_t only seems to exist for C++ via cstddef, where it's mapped to double.

That means I can't use max_align_t in CPython? Well, I can work around it.
But I was under the impression that MSVC did support C11. I can start the discussion to add max_align_t (and possibly alignof?) as exceptions to PEP 7. Do you know of any other C11 required features MSVC (or our configuration of it) doesn't support?

…e flags Now is not the time to fix that.

🤖 New build scheduled with the buildbot fleet by @encukou for commit ec9d5a8 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

The TraceRefs buildbot failure:

Modules/gcmodule.c:450: visit_decref: Assertion "!_PyObject_IsFreed(op)" failed
Enable tracemalloc to get the memory block allocation traceback

is likely caused by gh-103621.

void* cannot participate in address arithmetics. You need to cast both data_ptr and instance to char * to get distance in bytes.

🤖 New build scheduled with the buildbot fleet by @encukou for commit 402ecc6 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

Smaller objects might not be aligned to ALIGNOF_MAX_ALIGN_T. The offsets calculated for PEP 697 should be aligned, though.

🤖 New build scheduled with the buildbot fleet by @encukou for commit db5d49b 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

Very nice!

Sorry for the late review; it took some time to dig through everything.

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

Thank you for the thorough review!
This should address the remaining comments.

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.)

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 <oleg@arhadthedev.net>

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.

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 alignof(max_align_t): https://discuss.python.org/t/requiring-compilers-c11-standard-mode-to-build-cpython/26481

pythongh-103509 : PEP 697 -- Limited C API for Extending Opaque Types

a9679fa

encukou requested review from a team and markshannon as code owners April 13, 2023 14:56

bedevere-bot mentioned this pull request Apr 13, 2023

PEP 697 -- Limited C API for Extending Opaque Types #103509

Closed

bedevere-bot added the awaiting core review label Apr 13, 2023

Add a blurb

b8dc072

encukou marked this pull request as draft April 13, 2023 15:01

erlend-aasland self-requested a review April 16, 2023 21:25

encukou added 2 commits April 17, 2023 14:12

Fix PyMember_SetOne error return (thanks MSVC!)

1fc5c52

Add What's New entry

075ca51

encukou added 6 commits April 19, 2023 14:48

Fix warning in tests

439de5d

Work around lack of alignof & max_align_t

291731b

Use the same Sphinx role for Py_TPFLAGS_ITEMS_AT_END as for other typ…

2ba8084

…e flags Now is not the time to fix that.

Add ALIGNOF_MAX_ALIGN_T to configure & PC/pyconfig.h

b03d431

Use ALIGNOF_MAX_ALIGN_T

ec9d5a8

Fix typo

5cab814

encukou marked this pull request as ready for review April 20, 2023 08:52

encukou requested a review from corona10 as a code owner April 20, 2023 08:52

encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 20, 2023

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 20, 2023

encukou marked this pull request as draft April 20, 2023 09:41

encukou added 3 commits April 20, 2023 15:53

Don't include <stdalign.h>, it's not always available on Windows

3ade585

Define ALIGNOF_MAX_ALIGN_T with long double if it's not available

986bf26

tests: Compute data_offset in C to avoid overflow issues

e7838ee

arhadthedev requested changes Apr 21, 2023

View reviewed changes

Comment thread Modules/_testcapi/heaptype_relative.c Outdated

Comment thread Modules/_testcapi/heaptype_relative.c Outdated

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 27, 2023

encukou added 2 commits April 28, 2023 13:36

Don't rely on PyObject* being aligned to ALIGNOF_MAX_ALIGN_T

1701688

Smaller objects might not be aligned to ALIGNOF_MAX_ALIGN_T. The offsets calculated for PEP 697 should be aligned, though.

Merge in main branch

db5d49b

encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 28, 2023

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 28, 2023

erlend-aasland reviewed Apr 28, 2023

View reviewed changes

encukou and others added 7 commits May 2, 2023 15:15

Apply suggestions from code review

9d9911d

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

Raise TypeError on missing flag

06bcf5b

Wrap new tests in a class

0b69748

Use PyModule_AddIntMacro in tests

33c5258

Test failure of extending non-ITEMS_AT_END variable-sized types

88dade7

Test error cases around members

8720b25

Merge in the main branch

0474e69

encukou requested a review from arhadthedev May 2, 2023 15:20

arhadthedev reviewed May 2, 2023

View reviewed changes

Comment thread Doc/c-api/object.rst

Comment thread Doc/c-api/object.rst Outdated

Comment thread Doc/c-api/object.rst

Comment thread Doc/c-api/type.rst Outdated

Apply suggestions from code review

a975de9

Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>

encukou commented May 3, 2023

View reviewed changes

Comment thread Doc/c-api/object.rst

Comment thread Doc/c-api/object.rst

Merge in the main branch

b9ddf21

arhadthedev approved these changes May 3, 2023

View reviewed changes

encukou merged commit cd9a56c into python:main May 4, 2023

bedevere-bot removed the awaiting core review label May 4, 2023

encukou deleted the extend-opaque-sq branch May 4, 2023 07:56

hugovk mentioned this pull request Dec 29, 2023

gh-101100: Fix Sphinx warnings in c-api/structures.rst #113564

Merged

gh-103509: PEP 697 -- Limited C API for Extending Opaque Types by encukou · Pull Request #103511 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-103509: PEP 697 -- Limited C API for Extending Opaque Types - #103511

gh-103509: PEP 697 -- Limited C API for Extending Opaque Types#103511encukou merged 29 commits intopython:mainpython/cpython:mainfrom encukou:extend-opaque-sqencukou/cpython:extend-opaque-sqCopy head branch name to clipboard

Conversation

encukou commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

encukou commented Apr 17, 2023

Uh oh!

zooba commented Apr 17, 2023

Uh oh!

encukou commented Apr 19, 2023

Uh oh!

bedevere-bot commented Apr 20, 2023

Uh oh!

arhadthedev commented Apr 20, 2023

Uh oh!

arhadthedev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Apr 27, 2023

Uh oh!

bedevere-bot commented Apr 28, 2023

Uh oh!

erlend-aasland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

encukou commented May 2, 2023

Uh oh!

erlend-aasland commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arhadthedev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

encukou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

encukou commented May 4, 2023

Uh oh!

erlend-aasland commented May 4, 2023

Uh oh!

encukou commented May 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gh-103509: PEP 697 -- Limited C API for Extending Opaque Types#103511
encukou merged 29 commits into
python:mainfrom
encukou:extend-opaque-sq

encukou commented Apr 13, 2023 •
edited
Loading

erlend-aasland commented May 2, 2023 •
edited
Loading