Skip to content

gh-121249: Support _Complex types in the struct module - #121613

Merged
vstinner merged 12 commits into
python:mainfrom
skirpichev:complex-in-struct-module-121249
Oct 7, 2024
Merged

gh-121249: Support _Complex types in the struct module#121613
vstinner merged 12 commits into
python:mainfrom
skirpichev:complex-in-struct-module-121249

Conversation

@skirpichev

@skirpichevskirpichev commented Jul 11, 2024

Copy link
Copy Markdown
Member

@ZeroIntensityZeroIntensity left a comment

Copy link
Copy Markdown
Member

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.

Comment threadDoc/library/struct.rst Outdated
Comment threadDoc/library/struct.rst
Comment threadMisc/NEWS.d/next/Library/2024-07-10-08-13-34.gh-issue-121249.W9Gd09.rst Outdated
Comment threadMisc/NEWS.d/next/Library/2024-07-10-08-13-34.gh-issue-121249.W9Gd09.rst Outdated
Comment threadModules/_struct.c
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
skirpichev

This comment was marked as outdated.

@ZeroIntensity

Copy link
Copy Markdown
Member

Lets leave this to core devs.

This is why things get discussed on discourse first 😉

Comment threadDoc/library/struct.rst Outdated
Comment threadLib/test/test_struct.py Outdated

@ZeroIntensityZeroIntensity left a comment

Copy link
Copy Markdown
Member

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

@picnixzpicnixz left a comment

Copy link
Copy Markdown
Member

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.

Comment threadDoc/library/struct.rst Outdated
Comment threadDoc/library/struct.rst Outdated
@skirpichev
skirpichevforce-pushed the complex-in-struct-module-121249 branch from 7806d52 to fec6be6CompareJuly 13, 2024 04:12
@skirpichev

Copy link
Copy Markdown
MemberAuthor

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

Comment threadLib/test/test_struct.py Outdated

@picnixzpicnixz left a comment

Copy link
Copy Markdown
Member

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.

@skirpichev

Copy link
Copy Markdown
MemberAuthor

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

Comment threadLib/test/test_struct.py Outdated
Comment threadLib/test/test_struct.py Outdated
Comment threadModules/_struct.c Outdated
Comment threadModules/_struct.c Outdated
Comment threadModules/_struct.c Outdated
Comment threadModules/_struct.c Outdated
Comment threadModules/_struct.c Outdated

@vstinnervstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@vstinner

Copy link
Copy Markdown
Member

Merged, thanks.

@skirpichev
skirpichev deleted the complex-in-struct-module-121249 branch October 7, 2024 11:53
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@skirpichev@ZeroIntensity@vstinner@picnixz