-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-130317: fix PyFloat_Pack/Unpack[24] for NaN's with payload #130452
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
Merged
Merged
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
971fd98
gh-130317: fix PyFloat_Pack2/Unpack2 for NaN's with payload
skirpichev b05720a
Merge branch 'master' into fix-nan-packing/130317
skirpichev f513b14
more tests in test_capi, fix also Pack/Unpack4
skirpichev 218f6b3
+ different fix for Pack/Unpack2 (works as for floats)
skirpichev a7f5ed9
blacklist on win32
skirpichev c7c08ff
cleanup and comments
skirpichev de7b6aa
+ enable test_pack_unpack_roundtrip_nans on win32 for qNaN's
skirpichev e6c1c12
+1
skirpichev ac7bdcb
address review: warning on Windows
skirpichev b461b81
XXX try to revert win32 WA
skirpichev cbfbf05
Revert "XXX try to revert win32 WA"
skirpichev 6a5dc1a
Merge branch 'master' into fix-nan-packing/130317
skirpichev 86ab7c3
Merge branch 'main' into fix-nan-packing/130317
skirpichev 32bdeed
revert redundant test
skirpichev 773f51e
rename test
skirpichev b27c916
XXX revert win32 wa
skirpichev 4dc5697
+1
skirpichev 6f71e34
+1
skirpichev 3e5a211
revert
skirpichev 092d729
fixup value on win32
skirpichev 6137c75
cleanup & comments
skirpichev 00dfd66
address review: redo helper
skirpichev b7f727c
Apply suggestions from code review
skirpichev 13e8310
restore comment
skirpichev bcf54f6
address review: ensure code works with strict aliasing
skirpichev 9d20633
Apply suggestions from code review
skirpichev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 changes: 4 additions & 0 deletions
4
Misc/NEWS.d/next/Library/2025-02-22-13-07-06.gh-issue-130317.tnxd0I.rst
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
Fix :c:func:`PyFloat_Pack2` and :c:func:`PyFloat_Unpack2` for NaN's with | ||
payload. This corrects round-trip for :func:`struct.unpack` and | ||
:func:`struct.pack` in case of the IEEE 754 binary16 "half precision" type. | ||
Patch by Sergey B Kirpichev. |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
100 tests should be enough to validate the implementation, no?
1000 tests might be a little bit too slow, I don't think that it's worth it.
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.
Up to you, the test looks instantaneous on my system. 0.3sec vs 0.03. Where the threshold?
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.
I don't think speed is really an issue here, but having the potential for 6000 failed test reports seems like major overkill. I think 10 would actually be plenty for this loop.