Skip to content

Conversation

@skirpichev
Copy link
Member

@skirpichevskirpichev commented Jul 11, 2024

Copy link
Member

@ZeroIntensityZeroIntensity left a 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.

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
skirpichev

This comment was marked as outdated.

@ZeroIntensity
Copy link
Member

Lets leave this to core devs.

This is why things get discussed on discourse first 😉

Copy link
Member

@ZeroIntensityZeroIntensity left a 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).

Copy link
Member

@picnixzpicnixz left a 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.

@skirpichevskirpichevforce-pushed the complex-in-struct-module-121249 branch from 7806d52 to fec6be6CompareJuly 13, 2024 04:12
@skirpichev
Copy link
MemberAuthor

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

Copy link
Member

@picnixzpicnixz left a 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.

@skirpichev
Copy link
MemberAuthor

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

Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

LGTM

@vstinner
Copy link
Member

Merged, thanks.

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