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: Python 3.11 uses C99 and C11 #2309

Merged
merged 3 commits into from
Feb 24, 2022
Merged

PEP 7: Python 3.11 uses C99 and C11 #2309

merged 3 commits into from
Feb 24, 2022

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Feb 7, 2022

No description provided.

@vstinner
Copy link
Member Author

vstinner commented Feb 7, 2022

Update PEP 7 to reflect what's being used in practice: https://docs.python.org/dev/whatsnew/3.11.html#build-changes

  • Building Python now requires a C99 <math.h> header file providing the following functions: copysign(), hypot(), isfinite(), isinf(), isnan(), round(). (Contributed by Victor Stinner in bpo-45440.)

  • Building Python now requires a C99 <math.h> header file providing a NAN constant, or the __builtin_nan() built-in function. If a platform does not support Not-a-Number (NaN), the Py_NO_NAN macro can be defined in the pyconfig.h file. (Contributed by Victor Stinner in bpo-46640.)

IMO it's no longer worth it to go into details of which C99 features are used or not.

See https://bugs.python.org/issue46640 for @encukou's request about updating the PEP.

I'm only aware of one C99 feature which is causing troubles: https://bugs.python.org/issue40120 This issue is still being discussed, I prefer to not mention it in the PEP.

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two on grammar

A

pep-0007.txt Outdated Show resolved Hide resolved
pep-0007.txt Outdated Show resolved Hide resolved
@tiran
Copy link
Member

tiran commented Feb 7, 2022

We cannot require full C11 compatibility becase we have to special case MSVC. MSVC does not implement C11 features. Instead MSVC provides substitutes for APIs. For example MSVC has interlocked variable access instead of stdatomic.

GCC < 4.8 and AFAIK clang < 4.0 are lacking stdatomic support. I'm unsure about ICC. I found second hand information that ICC < 18 has incomplete stdatomic.h. I asked somebody to look into AIX xlc.

@vstinner
Copy link
Member Author

vstinner commented Feb 7, 2022

We cannot require full C11 compatibility

My change doesn't require C11, but C99.

@vstinner
Copy link
Member Author

vstinner commented Feb 7, 2022

Two on grammar

Fixed.

@vstinner
Copy link
Member Author

vstinner commented Feb 7, 2022

cc @gpshead @encukou @pablogsal: Is it ok to require C99 to build Python 3.11? I don't think that we have to go into the details of which C99 features are used by Python, do you?

@gvanrossum
Copy link
Member

Where was the discussion leading to this PEP change? It shouldn't be happening on this PR -- the peps repo is not for discussions about contents.

@vstinner
Copy link
Member Author

vstinner commented Feb 7, 2022

@gvanrossum:

Where was the discussion leading to this PEP change? It shouldn't be happening on this PR -- the peps repo is not for discussions about contents.

https://bugs.python.org/issue46640#msg412728

@gvanrossum
Copy link
Member

gvanrossum commented Feb 7, 2022 via email

@vstinner
Copy link
Member Author

vstinner commented Feb 7, 2022

That discussion is far from conclusive. Please escalate to python-dev.

Oh, I didn't expect this change to be controversial. Anyway, I created a thread on python-dev: https://mail.python.org/archives/list/python-dev@python.org/thread/J5FSP6J4EITPY5C2UJI7HSL2GQCTCUWN/

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current version LGTM from a PEP editor perspective, though you may need to modify it pending the outcome of the Python-Dev discussion (C11? Mention C99 features dropped in C11? Explicitly call out features unsupported in MSVC? Clarify that optional features are not supported? Add optional atomic support? etc)

@CAM-Gerlach CAM-Gerlach changed the title Python 3.11 uses C99 PEP 7: Require C99 for Python 3.11+ Feb 16, 2022
@vstinner vstinner changed the title PEP 7: Require C99 for Python 3.11+ PEP 7: Python 3.11 uses C99 and C11 Feb 23, 2022
@vstinner
Copy link
Member Author

I created a thread on python-dev: https://mail.python.org/archives/list/python-dev@python.org/thread/J5FSP6J4EITPY5C2UJI7HSL2GQCTCUWN/

There is a consensus on using C99 and a willingness to even use some C11 feature, so I updated my PR for that.

@tiran @mdickinson: Does it look good to you?

pep-0007.txt Outdated
Comment on lines 34 to 37
* Python 3.11 and newer versions use C99 in the public C API and use a
subset of C11 in Python internals. The public C API should be
compatible with C++. The C11 subset are features supported by GCC 8.5,
clang 8.0, and MSVC of Visual Studio 2017.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The public C API should be compatible with C11 as well.
C11 made several features present in C99 optional (most notably variable-length arrays, MSVC doesn't have). Those shouldn't be used in the API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let me try something simpler: "Python 3.11 and newer versions use C11 without optional features". Is it better? (see the updated PR)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you want to add a reference for that, but a simple overview is:
https://en.wikipedia.org/wiki/C11_%28C_standard_revision%29#Optional_features

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That indeed seems helpful

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One grammar correction; otherwise LGTM from a textual perspective. Adding a link to C11 seems to be a good idea, though.

pep-0007.txt Outdated

* Python versions greater than or equal to 3.6 use C89 with several
select C99 features:
* Python 3.6 to 3.10 uses C89 with several select C99 features:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Python 3.6 to 3.10 uses C89 with several select C99 features:
* Python 3.6 to 3.10 use C89 with several select C99 features:

Plural agreement error (grammar)

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once CAM approves this can go in. Thanks for pushing for this!

pep-0007.txt Outdated Show resolved Hide resolved
pep-0007.txt Outdated Show resolved Hide resolved
@AA-Turner
Copy link
Member

The latest version of the PR says "C11 without optional features" -- ideally the PR title should be updated before merging too for an accurate commit message.

A

@vstinner
Copy link
Member Author

The latest version of the PR says "C11 without optional features" -- ideally the PR title should be updated before merging too for an accurate commit message.

I changed the commit title. GitHub keeps the original commit title. When I click on merge, it uses the commit title, but not the GitHub PR title... I hate that part of GitHub. Gerrit is way better for that, it's possible to review the commit message.

@vstinner vstinner merged commit e846b70 into python:main Feb 24, 2022
@vstinner vstinner deleted the pep7 branch February 24, 2022 23:13
@vstinner
Copy link
Member Author

Merged, thanks for the reviews.

@gvanrossum
Copy link
Member

The latest version of the PR says "C11 without optional features" -- ideally the PR title should be updated before merging too for an accurate commit message.

I changed the commit title. GitHub keeps the original commit title. When I click on merge, it uses the commit title, but not the GitHub PR title... I hate that part of GitHub. Gerrit is way better for that, it's possible to review the commit message.

Must be your git(hub) tooling. When I merge (in the browser) I get to edit the commit title and message.

@vstinner
Copy link
Member Author

Must be your git(hub) tooling. When I merge (in the browser) I get to edit the commit title and message.

Yes. I also get a form to edit the PR title. But in my experience, if I modify the commit title in Git and then use git push --force, the GitHub PR title is not updated. And when I merge a PR, the edit form gives me the title from Git as expected.

It's just that I'm too lazy sometimes to update them :-)

Anyway, I modified the merged commit title to omit C99 :-)

@CAM-Gerlach
Copy link
Member

Yeah, the PR title is auto-filled from the initial commit message if the PR branch only has one commit, but otherwise independent of the commit message, since PRs can have any number of commits and it wouldn't really make sense to arbitrarily lock it to one of them. The PR title can be changed at any time, and the commit message can likewise be modified at will when merging.

Anyway, I modified the merged commit title to omit C99 :-)

On main? 😱

@vstinner
Copy link
Member Author

On main? scream

I used the merge form to clean up the commit message. See the final commit: e846b70

@CAM-Gerlach
Copy link
Member

Understood, thanks. I apologize for the confusion; my fault there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants