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

PEP 7: Public C API should be compatible with C99 #3862

Closed
wants to merge 2 commits into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jul 14, 2024


📚 Documentation preview 📚: https://pep-previews--3862.org.readthedocs.build/

@colesbury
Copy link
Contributor

Huh? The public headers are not C99 or C++03 compatible:

  • The default build (NOT the free-threading build) uses an unnamed union struct _object (C11, C++, or extension)
  • pyatomic.h requires C11 atomics, C++11, or GNU or MSVC intrinsics

@warsaw
Copy link
Member

warsaw commented Jul 15, 2024

@colesbury are you saying that mimalloc is a blocker for this PR?

@colesbury
Copy link
Contributor

@warsaw, mimalloc is not part of the public headers so I don't think it's relevant.

The pyatomic.h header is used by the refcounting functions in the free-threaded build. These are mostly static inline functions exposed by the public C API headers. There isn't a ISO C99 replacement here.

The PyObject struct definition relies on either C11 or non-standard extensions. Changing this to a non-anonymous union risk breaking backwards compatibility.

If the intention is that new code in public headers should be ISO C99 and ISO C++03 compatible, that's probably fine, but I think it'd be worth saying so explicitly in the PEP.

(See also capi-workgroup/decisions#30)

@vstinner
Copy link
Member Author

@colesbury: I updated the PEP to apply @encukou's suggestion: capi-workgroup/decisions#30 (comment)

@vstinner
Copy link
Member Author

vstinner commented Aug 1, 2024

cc @encukou

@encukou
Copy link
Member

encukou commented Aug 2, 2024

Looks good to me, though the PEP authors have the final say here.
Long-term, let's move this out of the style guide altogether, into the C API PEP. This is not a rule you should break if it “would make the code less readable” :)

@vstinner
Copy link
Member Author

vstinner commented Aug 2, 2024

@warsaw @gvanrossum: Are you ok with these changes?

@gvanrossum
Copy link
Member

I've lost track of the discussion -- why can't we just say "we require C11"? (And the equivalent C++, which is what?) Maybe the reason for this should be spelled out in the PEP so it will be easier to revise in the future.

@warsaw
Copy link
Member

warsaw commented Aug 2, 2024

Good question @gvanrossum . Also, I wonder if the "C dialect" section could be better represented as a table, with Python version along one dimension, and C dialect, C++ dialect, and additional notes along the other?

@encukou
Copy link
Member

encukou commented Aug 3, 2024

This PR is meant to document/formalize the status quo, not to be progressive.

Practically, in header code, the intersection of C99, C++09, and C11-without-optional-features is very close to “C89 with several select C99 features” we had for 3.6-3.10. But it's much easier to test.
Whenever we break compatibility here, someone complains, and in each individual case it's usually easier to revert than to push for C11.

There was a discussion about this on Discourse; also see the hoops we currently jump through to have a C11 feature in the headers.

Enabling a compiler feature changes the behaviour of my entire program, not just the CPython headers. As it turns out, there exist some huge beasts that use Python as a library, and are quite sensitive to compiler options. (Steve might nowadays be free to say what his motivating example was? Just to fuel speculation, Python in MS Excel was announced shortly after the discussion.)

@gvanrossum
Copy link
Member

Just to fuel speculation, Python in MS Excel was announced shortly after the discussion.

I don't think that's it -- in that architecture Python runs in a separate container. But I trust there are Microsoft projects not owned by Steve that are very sensitive to this stuff and unwilling/unable to change.

OTOH Victor's argument sounds more like speculation (exactly what Greg challenged earlier in that thread).

I am still uncomfortable with leaving the list of C11 features we use beyond C99/C++03 unspecified. I think we should either bite the bullet and stop using those, or otherwise document a timeline (say, 5 years) for changing the required standard level to C11/C++11 without optional features (or using the optional features only when they are available).

As the PR is currently phrased, it doesn't give enough (enforceable) guidelines to either the users of the public C API or its maintainers.

@encukou
Copy link
Member

encukou commented Aug 3, 2024

I agree that we should switch to C11 eventually. Let's have that discussion again; you mainly need to convince Steve.
(But if we document a timeline so that users can prepare for a migration, let's not put it in a C style guide...)

As the PR is currently phrased, it doesn't give enough (enforceable) guidelines to either the users of the public C API or its maintainers.

AFAIK, PEP 7 is meant for the maintainers. Users expect their builds to not break with new Python versions; this is how we give them that.

The rules are already enforced -- they're tested [c99, c++03], and regressions that aren't caught by the tests have been reverted (since we don't want to break users).

I am still uncomfortable with leaving the list of C11 features we use beyond C99/C++03 unspecified.

That would mainly be useful for users. Those can try compiling <Python.h> with whatever compiler/flags they need.
If we want to promise something to users, let's do that separately from guidelines for core devs.

@vstinner
Copy link
Member Author

vstinner commented Aug 5, 2024

I created a separated PR to require C11 and C++11: #3896

@vstinner
Copy link
Member Author

vstinner commented Sep 3, 2024

The discussion capi-workgroup/decisions#30 is still on-going, I prefer to close my PR for now.

@vstinner vstinner closed this Sep 3, 2024
@vstinner vstinner deleted the pep7 branch September 3, 2024 12:15
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.

5 participants