Skip to content

gh-61103: don't use native complex types in ctypes#133237

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.

Sign up for GitHub

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

Jump to bottom
Merged
encukou merged 6 commits into from
May 5, 2025
Merged

gh-61103: don't use native complex types in ctypes #133237

encukou merged 6 commits into from
May 5, 2025

Conversation

@skirpichev
Copy link
Member

@skirpichevskirpichev commented May 1, 2025•
edited
Loading

  • drop _complex.h header
  • use appropriate real arrays to replace complex types

📚 Documentation preview 📚: https://cpython-previews--133237.org.readthedocs.build/

Note, that Py_FFI_SUPPORT_C_COMPLEX check configure check imply support for complex types. So, Py_HAVE_C_COMPLEX check is redundant. Though, I'm not sure if it worth removing.

@skirpichev
pythongh-121249: make complex types available unconditionally in the …
66d7e54
…struct module Each complex type interpreted as an array type containing exactly two elements of the corresponding real type (real and imaginary parts, respectively).
@bedevere-appbedevere-appbot mentioned this pull request May 1, 2025
Closed
@skirpichev
Copy link
MemberAuthor

skirpichev commented May 1, 2025

This is on top of #132864

CC @dalcinl

@skirpichev
pythongh-61103: don't use native complex types to implement F/D/G in …
6a20faf
…ctypes * drop _complex.h header * use appropriate real arrays to replace complex types
@skirpichevskirpichevforce-pushed the no_complex.h/61103 branch from efbe0bb to 6a20fafCompareMay 1, 2025 04:32
@skirpichevskirpichev marked this pull request as ready for review May 1, 2025 05:17
@skirpichevskirpichev changed the title gh-61103: don't use native complex types to implement F/D/G in ctypesgh-61103: don't use native complex types in ctypesMay 1, 2025
dalcinl
dalcinl reviewed May 1, 2025
View reviewed changes
@dalcinl
Copy link
Contributor

dalcinl commented May 1, 2025•
edited
Loading

@skirpichev Thanks, it looks great. IMHO, and despite your doubts about worthiness, this should be merged. Things are simpler now, you removed lots of code, and a private header.

Minor nit: I think the complex types logic that got added to configure.ac can be safely removed.Needed for ctypes tests.

dalcinl
dalcinl reviewed May 1, 2025
View reviewed changes
@skirpichev@dalcinl
Update Modules/_struct.c
8bbed73
Co-authored-by: Lisandro Dalcin <dalcinl@gmail.com>
encukou
encukou reviewed May 1, 2025
View reviewed changes
@skirpichev@encukou
Apply suggestions from code review
f1fa51b
Co-authored-by: Petr Viktorin <encukou@gmail.com>
@skirpichevskirpichev requested review from dalcinl and encukouMay 1, 2025 13:39
@skirpichev@StanFromIreland
Update Misc/NEWS.d/next/Library/2025-04-25-16-20-49.gh-issue-121249.u…
071d57e
…ue2nK.rst Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
@encukouencukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 2, 2025
@bedevere-bot
Copy link

bedevere-bot commented May 2, 2025

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

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133237%2Fmerge

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

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 2, 2025
encukou
encukou reviewed May 2, 2025
View reviewed changes
Copy link
Member

@encukouencukou left a comment

Choose a reason for hiding this comment

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

LGTM, merge if the buildbots don't complain.

@skirpichev
Copy link
MemberAuthor

skirpichev commented May 2, 2025

@encukou, this pr was on top of #132864, which already tested with buildbots. If we are going to merge this, I would prefer to do it via two separate commits.

@encukou
Copy link
Member

encukou commented May 2, 2025

Ah, I missed that note, sorry!
Could you add the latest wording changes there?

@skirpichevskirpichev mentioned this pull request May 2, 2025
Merged
@skirpichev
Copy link
MemberAuthor

skirpichev commented May 2, 2025

Could you add the latest wording changes there?

Done. See also minor docs correction in #133249

@skirpichev
Copy link
MemberAuthor

skirpichev commented May 2, 2025

Failure in test_frame.py on s390x seems unrelated.

@skirpichev
Copy link
MemberAuthor

skirpichev commented May 4, 2025

All failures are related to test_frame.py.

@encukou, I left here only one change, related to the struct module (cosmetic change in _Alignof operands), as it was related to code. I hope it's ok.

@skirpichev
Copy link
MemberAuthor

skirpichev commented May 4, 2025

@dalcinl, let us know if you think that linked issue should stay open after merging this.

@dalcinl
Copy link
Contributor

dalcinl commented May 4, 2025

@skirpichev I think you can close it. AFAICT, all concerns have been addressed. Many thanks for your hard work in implementing all these changes.

encukou
encukou approved these changes May 5, 2025
View reviewed changes
Copy link
Member

@encukouencukou left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@encukouencukou merged commit 8d0e07e into python:mainMay 5, 2025
45 checks passed
@skirpichevskirpichev deleted the no_complex.h/61103 branch May 5, 2025 09:42
Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
@skirpichev@dalcinl
@encukou@StanFromIreland@Pranjal095
pythongh-61103: don't use C _Complex types to implement F/D/G in ctyp…
007c71e
…es (pythonGH-133237) According to the C standard, the memory representation of _Complex types is equivalent to 2-element arrays. Unlike _Complex, arrays are always available. - drop _complex.h header - use appropriate real arrays to replace complex types Co-authored-by: Lisandro Dalcin <dalcinl@gmail.com> Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
@junkmdjunkmd mentioned this pull request Jul 21, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StanFromIrelandStanFromIrelandStanFromIreland left review comments

@encukouencukouencukou approved these changes

@erlend-aaslanderlend-aaslandAwaiting requested review from erlend-aaslanderlend-aasland is a code owner

+2 more reviewers

@dalcinldalcinldalcinl approved these changes

@DangNhutNguyenDangNhutNguyenDangNhutNguyen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@skirpichev@dalcinl@bedevere-bot@encukou@StanFromIreland@DangNhutNguyen