8000 gh-61103: support double complex (_Complex) type in ctypes by skirpichev · Pull Request #120894 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-61103: support double complex (_Complex) type in ctypes #120894

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
merged 57 commits into from
Jul 1, 2024
Merged

gh-61103: support double complex (_Complex) type in ctypes #120894

merged 57 commits into from
Jul 1, 2024

Conversation

skirpichev
Copy link
Contributor
@skirpichev skirpichev commented Jun 23, 2024 •
edited
Loading

Example:

>>> import ctypes
>>> libm = ctypes.CDLL('libm.so.6')
>>> libm.clog.argtypes = [ctypes.c_double_complex]
>>> libm.clog.restype = ctypes.c_double_complex
>>> libm.clog(1+1j)
(0.34657359027997264+0.7853981633974483j)

ctypes.c_double_complex is available only if compiler does support complex arithmetic (Annex G).

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

Notes for reviewers:

  • It seems, most compilers implement this optional feature of C11+. (Though neither does this correctly.) Maybe we should require one?
  • only one complex type was added; if this looks good - I can add the rest (float complex and long double complex) in this pr or in other.
  • c_double_complex._type_ chosen to be "C". Maybe we should allow instead multiple chars to specify types (e.g. "Cd" for double complex)?
  • this pr doesn't include changes in the struct module. Perhaps, I should add support for complex type here as well?
  • if test_complex_round_trip() does make sense for reviewers, here is a simple refactoring to avoid adding yet another assertFloatsAreIdentical() helper: gh-121039: add Floats/ComplexesAreIdenticalMixin to test.support.testcase #121071

@skirpichev
pythongh-61103: support double complex (_Complex) type in ctypes
c9b872f 
Example:
```pycon
>>> import ctypes
>>> ctypes.__STDC_IEC_559_COMPLEX__
1
>>> libm = ctypes.CDLL('libm.so.6')
>>> libm.clog.argtypes = [ctypes.c_double_complex]
>>> libm.clog.restype = ctypes.c_double_complex
>>> libm.clog(1+1j)
(0.34657359027997264+0.7853981633974483j)
```

``ctypes.__STDC_IEC_559_COMPLEX__`` is ``0`` if compiler doesn't support
complex arithmetic (Annex G).
@bedevere-app bedevere-app bot mentioned this pull request Jun 23, 2024
Closed
nineteendo
nineteendo reviewed Jun 23, 2024
View reviewed changes
skirpichev and others added 2 commits June 23, 2024 09:00
@skirpichev @nineteendo
Apply suggestions from code review
c296fb3 
Co-authored-by: Nice Zombies <nineteendo19d0@gmail.com>
@skirpichev
+ support _Complex in c-analyzer
7453ca5
@skirpichev skirpichev requested a review from nineteendo June 23, 2024 09:36
nineteendo
nineteendo reviewed Jun 23, 2024
View reviewed changes
@skirpichev @nineteendo
Apply suggestions from code review
bf81682 
Co-authored-by: Nice Zombies <nineteendo19d0@gmail.com>
picnixz
picnixz reviewed Jun 23, 2024
View reviewed changes
@picnixz
Copy link
Member
picnixz commented Jun 23, 2024 •
edited
Loading

I can add the rest (float complex and long double complex) in this pr or in other.

I'd prefer having a separate PR because we might need aliases in case sizeof(long double) == sizeof(double) (the same technique that is used for long long and long IIRC)

c_double_complex.type chosen to be "C". Maybe we should allow instead multiple chars to specify types (e.g. "Cd" for double complex)?

I think it might be indeed a good alternative, because otherwise you need to find good 1-char names for float complex and long double complex in the future, but I think Cf, Cd and Cl (or Cq for quad-precision) are good enough. But for that we need to update the fielddesc structure with an additional field that would be a "modifier" of some sort or allow the "code" to be more than just a single character.

this pr doesn't include changes in the struct module. Perhaps, I should add support for complex type here as well?

I'd say in a separate PR since there is a translation of ctypes format to PEP 3118 names (where complex numbers are represented by 'Z').

skirpichev and others added 2 commits June 23, 2024 14:47
@skirpichev @picnixz
Apply suggestions from code review
af2fdce 
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@skirpichev
+ allow _Complex specifier before float/double/etc
6d9d5ab
@skirpichev skirpichev force-pushed the ctypes-c_complex-61103 branch from 45f8544 to 013765e Compare June 23, 2024 13:07
@skirpichev skirpichev force-pushed the ctypes-c_complex-61103 branch from 013765e to f9c709c Compare June 23, 2024 17:33
@nineteendo
Copy link
Contributor

Use a force-push only to fix typo's or revert a merge.

@skirpichev
Copy link
Contributor Author

Use a force-push only to fix typo's or revert a merge.

Thank you for advice. Usually I don't do this. I hope this time force-push was OK, as it's restricted to unreviewed commits.

@nineteendo
Copy link
Contributor
nineteendo commented Jun 23, 2024 •
edited
Loading

It's less bad if no-one commented on those commits, but it's still more work to figure out what you changed.
It's also more distracting than a regular commit. (also, you don't need to quote the whole previous message)

@skirpichev
Address review:
ccd418d 
* add configure test for _Complex type
* define HAVE_C_COMPLEX
* move workarounds for buggy implementations to Module/_complex.h
@vstinner
Copy link
Member

!buildbot s390x SLES

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @vstinner for commit e9cab4a 🤖

The command will test the builders whose names match following regular expression: s390x SLES

The builders matched are:

  • s390x SLES PR

@skirpichev
Copy link
Contributor Author
skirpichev commented Jun 30, 2024 •
edited
Loading

Well, one failure seems to be related:
https://buildbot.python.org/all/#/builders/1262/builds/13/steps/3/logs/stdio
Edit (similar one):
https://buildbot.python.org/all/#/builders/855/builds/119

Looking on the libffi sources, it seems that FFI_TYPE_COMPLEX is available unconditionally, even if platform doesn't support interfaces to _Complex (for sparc it works since v3.3). We should check FFI_TARGET_HAS_COMPLEX_TYPE instead.

I'll update PR when build finish.

@vstinner
Copy link
Member

!buildbot AMD64 Fedora Rawhide PR

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @vstinner for commit 6d5bf66 🤖

The command will test the builders whose names match following regular expression: AMD64 Fedora Rawhide PR

The builders matched are:

  • AMD64 Fedora Rawhide PR

@vstinner
Copy link
Member

!buildbot AMD64 Fedora Rawhide PR

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @vstinner for commit 6d5bf66 🤖

The command will test the builders whose names match following regular expression: AMD64 Fedora Rawhide PR

The builders matched are:

  • AMD64 Fedora Rawhide PR

@vstinner
Copy link
Member

Oh, Rawhide failure is unrelated: "No space left on device".

@vstinner
Copy link
Member

!buildbot PPC64LE CentOS9 PR

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @vstinner for commit 6d5bf66 🤖

The command will test the builders whose names match following regular expression: PPC64LE CentOS9 PR

The builders matched are:

  • PPC64LE CentOS9 PR

@vstinner
Copy link
Member

!buildbot PPC64LE Fedora Stable Clang PR

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @vstinner for commit 6d5bf66 🤖

The command will test the builders whose names match following regular expression: PPC64LE Fedora Stable Clang PR

The builders matched are:

  • PPC64LE Fedora Stable Clang PR

@vstinner vstinner added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 1, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @vstinner for commit 6d5bf66 🤖

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 Jul 1, 2024
@vstinner vstinner merged commit 6988ff0 into python:main Jul 1, 2024
78 of 90 checks passed
@vstinner
Copy link
Member
vstinner commented Jul 1, 2024

Thanks @skirpichev, I merged your PR.

I checked remaining buildbot failures: they are unrelated to the change.

@skirpichev: @erlend-aasland has concerns about relative #include.

@skirpichev skirpichev deleted the ctypes-c_complex-61103 branch July 1, 2024 09:56
@skirpichev
Copy link
Contributor Author

Thanks for reviews and patience.

@erlend-aasland has concerns about relative #include.

Yep, here; I'll open an issue (I think that build failures in this pr already don't need this;)). There are other includes, besides _math.h.

@skirpichev skirpichev mentioned this pull request Jul 1, 2024
Open
Akasurde pushed a commit to Akasurde/cpython that referenced this pull request Jul 3, 2024
@skirpichev @nineteendo
@picnixz @vstinner @Akasurde
pythongh-61103: Support double complex (_Complex) type in ctypes (pyt…
704c35e 
…hon#120894)

Example:

```pycon
>>> import ctypes
>>> ctypes.__STDC_IEC_559_COMPLEX__
1
>>> libm = ctypes.CDLL('libm.so.6')
>>> libm.clog.argtypes = [ctypes.c_double_complex]
>>> libm.clog.restype = ctypes.c_double_complex
>>> libm.clog(1+1j)
(0.34657359027997264+0.7853981633974483j)
```

Co-authored-by: Nice Zombies <nineteendo19d0@gmail.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Victor Stinner <vstinner@python.org>
@skirpichev skirpichev mentioned this pull request Jul 10, 2024
Merged
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
@skirpichev @nineteendo
@picnixz @vstinner @noahbkim
pythongh-61103: Support double complex (_Complex) type in ctypes (pyt…
46a4f6e 
…hon#120894)

Example:

```pycon
>>> import ctypes
>>> ctypes.__STDC_IEC_559_COMPLEX__
1
>>> libm = ctypes.CDLL('libm.so.6')
>>> libm.clog.argtypes = [ctypes.c_double_complex]
>>> libm.clog.restype = ctypes.c_double_complex
>>> libm.clog(1+1j)
(0.34657359027997264+0.7853981633974483j)
```

Co-authored-by: Nice Zombies <nineteendo19d0@gmail.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Victor Stinner <vstinner@python.org>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@skirpichev @nineteendo
@picnixz @vstinner @estyxx
pythongh-61103: Support double complex (_Complex) type in ctypes (pyt…
b3f91a6 
…hon#120894)

Example:

```pycon
>>> import ctypes
>>> ctypes.__STDC_IEC_559_COMPLEX__
1
>>> libm = ctypes.CDLL('libm.so.6')
>>> libm.clog.argtypes = [ctypes.c_double_complex]
>>> libm.clog.restype = ctypes.c_double_complex
>>> libm.clog(1+1j)
(0.34657359027997264+0.7853981633974483j)
```

Co-authored-by: Nice Zombies <nineteendo19d0@gmail.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Victor Stinner <vstinner@python.org>
This was referenced Oct 7, 2024
Merged
Closed
Merged
@skirpichev skirpichev mentioned this pull request Jan 22, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

@picnixz picnixz picnixz approved these changes

@nineteendo nineteendo nineteendo left review comments

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@skirpichev @picnixz @nineteendo @vstinner @bedevere-bot @erlend-aasland
0