GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-121249: Support _Complex types in the struct module by skirpichev · Pull Request #121613 · python/cpython

Conversation

skirpichev commented Jul 11, 2024 •

edited by github-actions Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Issue: Add support for C99 complex type (_Complex) to the struct module #121249

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          pythongh-121249: Support _Complex types in the struct module

c60f4a4

bedevere-app Bot mentioned this pull request

Jul 11, 2024

Add support for C99 complex type (_Complex) to the struct module #121249

Closed

bedevere-app Bot added the awaiting review label

Jul 11, 2024

ZeroIntensity requested changes

Jul 11, 2024

View reviewed changes

ZeroIntensity left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nice PR! This looks like something that would have had to get approved on Discourse, though.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Doc/library/struct.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Doc/library/struct.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Misc/NEWS.d/next/Library/2024-07-10-08-13-34.gh-issue-121249.W9Gd09.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Misc/NEWS.d/next/Library/2024-07-10-08-13-34.gh-issue-121249.W9Gd09.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_struct.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot added awaiting core review and removed awaiting review labels

Jul 11, 2024


          Apply suggestions from code review

3aac6d1

Co-authored-by: Peter Bierma <[email protected]>

This comment was marked as outdated.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ZeroIntensity commented Jul 11, 2024

Copy link

Copy Markdown

Member

Lets leave this to core devs.

This is why things get discussed on discourse first 😉

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

skirpichev commented

Jul 12, 2024

View reviewed changes

Comment thread

Doc/library/struct.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

skirpichev commented

Jul 12, 2024

View reviewed changes

Comment thread

Lib/test/test_struct.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

ZeroIntensity approved these changes

Jul 12, 2024

View reviewed changes

ZeroIntensity left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This implementation LGTM.

Although, for core dev review, I'm not too sure about this behavior: if the compiler does not support C11 complex types, then the new format characters are disabled and raise an error with no further information.

I'm worried that this will lead to some bug reports later on wondering why E and C don't work on their system -- it would be ideal to raise a different error mentioning that the system doesn't support C11 (albeit, it would be more work to implement).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

picnixz reviewed

Jul 13, 2024

View reviewed changes

picnixz left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Some minor comments but I also agree that having an error without info for an unknown format if complex are not supported is a bit weird.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Doc/library/struct.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Doc/library/struct.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

skirpichev and others added 3 commits

July 13, 2024 05:31


          Update Doc/library/struct.rst

785396a


          Apply suggestions from code review

e5e04c8

Co-authored-by: Bénédikt Tran <[email protected]>


          raise a custom error on systems without native complex numbers

fec6be6

skirpichev force-pushed the complex-in-struct-module-121249 branch from 7806d52 to fec6be6 Compare

July 13, 2024 04:12

skirpichev commented Jul 13, 2024

Copy link

Copy Markdown

Member Author

Now on systems without _Complex type - a different error will be raised.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

picnixz reviewed

Jul 13, 2024

View reviewed changes

Comment thread

Lib/test/test_struct.py Resolved

Uh oh!

There was an error while loading. Please reload this page.


          address review: separate tests

caaef44

picnixz approved these changes

Jul 13, 2024

View reviewed changes

picnixz left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

On my side it's fine. I didn't check in details the functions bodies, but I assume that the tests should cover most of the cases. I think it remains to wait for Victor's and/or Serhiy's review.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

skirpichev reacted with thumbs up emoji

All reactions

👍 1 reaction

skirpichev added 2 commits

October 7, 2024 07:02


          Merge branch 'master' into complex-in-struct-module-121249

3256f9d


          + use ComplexesAreIdenticalMixin from test.support

4097aaf

skirpichev commented Oct 7, 2024

Copy link

Copy Markdown

Member Author

As it's a follow up of #120894, CC @vstinner

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner reviewed

Oct 7, 2024

View reviewed changes

Comment thread

Lib/test/test_struct.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/test/test_struct.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_struct.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_struct.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_struct.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_struct.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_struct.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

skirpichev and others added 4 commits

October 7, 2024 12:17


          Apply suggestions from code review

5fe6c35

Co-authored-by: Victor Stinner <[email protected]>


          Merge branch 'master' into complex-in-struct-module-121249

0a9240f


          + fixed typo in lp_double_complex()

4820f19


          address review: clean up useless type casts in the struct module

fbd189f

skirpichev requested a review from vstinner

October 7, 2024 10:23

vstinner approved these changes

Oct 7, 2024

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot added awaiting merge and removed awaiting core review labels

Oct 7, 2024

vstinner merged commit 7487db4 into python:main

Oct 7, 2024

bedevere-app Bot removed the awaiting merge label

Oct 7, 2024

vstinner commented Oct 7, 2024

Copy link

Copy Markdown

Member

Merged, thanks.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

skirpichev deleted the complex-in-struct-module-121249 branch

October 7, 2024 11:53

Reviewers

vstinner

vstinner approved these changes

picnixz

picnixz approved these changes

ZeroIntensity

ZeroIntensity approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

skirpichev commented Jul 11, 2024 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

ZeroIntensity commented Jul 11, 2024

Uh oh!

Uh oh!

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

picnixz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

skirpichev commented Jul 13, 2024

Uh oh!

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

skirpichev commented Oct 7, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 7, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

skirpichev commented Jul 11, 2024 •

edited by github-actions Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

picnixz left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.