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.

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

skirpichev
Copy link
Member

@skirpichev skirpichev commented May 1, 2025

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

…struct module

Each complex type interpreted as an array type containing exactly two
elements of the corresponding real type (real and imaginary parts,
respectively).
@skirpichev
Copy link
Member Author

This is on top of #132864

CC @dalcinl

…ctypes

* drop _complex.h header
* use appropriate real arrays to replace complex types
@skirpichev skirpichev force-pushed the no_complex.h/61103 branch from efbe0bb to 6a20faf Compare May 1, 2025 04:32
@skirpichev skirpichev marked this pull request as ready for review May 1, 2025 05:17
@skirpichev skirpichev requested a review from erlend-aasland as a code owner May 1, 2025 05:17
@skirpichev skirpichev changed the title gh-61103: don't use native complex types to implement F/D/G in ctypes gh-61103: don't use native complex types in ctypes May 1, 2025
@dalcinl
Copy link
Contributor

dalcinl commented May 1, 2025

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

Co-authored-by: Lisandro Dalcin <dalcinl@gmail.com>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
@skirpichev skirpichev requested review from dalcinl and encukou May 1, 2025 13:39
…ue2nK.rst

Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 2, 2025
@bedevere-bot
Copy link

🤖 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-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 2, 2025
Copy link
Member

@encukou encukou 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
Member Author

@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?

@skirpichev
Copy link
Member Author

Could you add the latest wording changes there?

Done. See also minor docs correction in #133249

@skirpichev
Copy link
Member Author

Failure in test_frame.py on s390x seems unrelated.

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.

6 participants