-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
gh-111140: Fix edge case in PyLong_AsNativeBytes where large negative longs may require an extra byte #116053
Conversation
zooba
commented
Feb 28, 2024
•
edited by bedevere-app
bot
Loading
edited by bedevere-app
bot
- Issue: C API: Consider adding public PyLong_AsByteArray() and PyLong_FromByteArray() functions #111140
…gative longs may require an extra byte
I've improved detection of the two edge cases, though for now there isn't a full solution for the positive input->MSB set output case. What I have tried though is returning a wildly different value from those cases and specifically testing for that, and it comes up just fine. So when we decide how we want to say "it's okay for positive inputs to set the MSB in the result", those TODOs can just come out and set |
Updated the I still need to add new tests. @encukou you are likely interested in the parameter change here |
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.
The feature looks good to me. (I haven't looked at the implementation yet though.)
The docs need another pass: change remaining endianness args, ensure all the text is in sync, consider making it clearer that -1 isn't a combinable “flag”.
*[rng.randrange(256) for _ in range(n - 1)] | ||
]) | ||
bytes_le = bytes_be[::-1] | ||
v = int.from_bytes(bytes_le, 'little') |
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.
Could you use subTest
here, to have the chosen value show up on failure?
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 know that subTest is a good idea for non-repeatable values? But I'll see if I can tweak the failure handling to include the value when not running verbose (currently it'll be printed in verbose mode).
I updated the failing fuzz test output to look more like this:
In verbose mode:
Thoughts? I don't think making it a subTest is helpful, since there's no reasonable way to request the test run with a particular value, or to efficiently request a value be skipped. |
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.
Thank you! This looks great; I did find a few nitpicks.
Also, please consider these changes to the docs: zooba#33 |
Thank you! |
…just *endianness* (pythonGH-116053)