Skip to content

bpo-45034: Fixes how upper limit is formatted for struct.pack("H", ...) #28178

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 6 commits into from
Sep 7, 2021
Merged

bpo-45034: Fixes how upper limit is formatted for struct.pack("H", ...) #28178

merged 6 commits into from
Sep 7, 2021

Conversation

@sobolevn
Copy link
Member

@sobolevn sobolevn commented Sep 5, 2021 •
edited
Loading

https://bugs.python.org/issue45034

It was:

struct.error: ushort format requires 0 <= number <= (0x7fff * 2 + 1)

It is now:

struct.error: ushort format requires 0 <= number <= 65535
mdickinson
mdickinson reviewed Sep 6, 2021
View reviewed changes
@mdickinson
Copy link
Member

mdickinson commented Sep 6, 2021

Thanks for the PR. I think we should extend the fix to np_short, too: those uses of Py_STRINGIFY never really made much sense.
mdickinson
mdickinson reviewed Sep 6, 2021
View reviewed changes
sobolevn and others added 2 commits September 6, 2021 11:21
@sobolevn @mdickinson
Update Modules/_struct.c
fba681c 
Co-authored-by: Mark Dickinson <dickinsm@gmail.com>
@sobolevn @mdickinson
Update Modules/_struct.c
9f285bb 
Co-authored-by: Mark Dickinson <dickinsm@gmail.com>
@sobolevn
Copy link
Member Author

sobolevn commented Sep 6, 2021

I think we should extend the fix to np_short, too: those uses of Py_STRINGIFY never really made much sense.

Will do! Thanks a lot for your help 👍
mdickinson
mdickinson approved these changes Sep 7, 2021
View reviewed changes
Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM; thank you! Do you want to add your name to the list in Misc/ACKS? (It's entirely optional.)
@sobolevn
Copy link
Member Author

sobolevn commented Sep 7, 2021

Do you want to add your name to the list in Misc/ACKS?

Why not? 😄 Added!

Thanks a lot for your help! 👍
serhiy-storchaka
serhiy-storchaka reviewed Sep 7, 2021
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

What about other integer and floating point types?
@mdickinson
Copy link
Member

mdickinson commented Sep 7, 2021

What about other integer and floating point types?

I think we can leave broader changes for a separate PR, if anyone feels inclined; this PR addresses the immediate issue that @stevendaprano was seeing, which was due to inappropriate uses of Py_STRINGIFY.
@sobolevn @serhiy-storchaka
Update Modules/_struct.c
c57e30c 
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@sobolevn
Copy link
Member Author

sobolevn commented Sep 7, 2021

Let's have an explicit cast then 🙂
@mdickinson mdickinson merged commit 8ca6b61 into python:main Sep 7, 2021
This was referenced Oct 13, 2022
Closed
Merged
@stevendaprano stevendaprano mentioned this pull request May 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants

@sobolevn @mdickinson @serhiy-storchaka @the-knights-who-say-ni @bedevere-bot