Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-111140: Fix edge case in PyLong_AsNativeBytes where large negative longs may require an extra byte#116053

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
encukou merged 19 commits intopython:mainfromzooba:gh-111140
Apr 5, 2024

Conversation

zooba
Copy link
Member

@zoobazooba commentedFeb 28, 2024•
edited by bedevere-appbot
Loading

@zoobazooba requested a review fromgpsheadFebruary 28, 2024 16:22
@gpsheadgpshead marked this pull request as draftFebruary 28, 2024 23:55
@zooba
Copy link
MemberAuthor

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 Ihave 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 setres = n (plus whatever extra condition is being tested).

@zoobazooba marked this pull request as ready for reviewMarch 19, 2024 22:14
@zooba
Copy link
MemberAuthor

zooba commentedMar 28, 2024•
edited
Loading

Updated theendianness argument toflags so we can pass more options in there. Existing calls still work, as the 0/1/-1 options are unchanged - the only changed existing test was looking for an error condition that no longer exists.

I still need to add new tests.

@encukou you are likely interested in the parameter change here

Copy link
Member

@encukouencukou left a 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 remainingendianness args, ensure all the text is in sync, consider making it clearer that -1 isn't a combinable “flag”.

zooba reacted with thumbs up emoji
*[rng.randrange(256) for _ in range(n - 1)]
])
bytes_le = bytes_be[::-1]
v = int.from_bytes(bytes_le, 'little')
Copy link
Member

Choose a reason for hiding this comment

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

Could you usesubTest here, to have the chosen value show up on failure?

Copy link
MemberAuthor

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

encukou reacted with thumbs up emoji
@zooba
Copy link
MemberAuthor

I updated the failing fuzz test output to look more like this:

0:00:00 Run 1 test sequentially0:00:00 [1/1] test_capitest test_capi failed -- Traceback (most recent call last):  File "D:\cpython\Lib\test\test_capi\test_long.py", line 663, in test_long_asnativebytes_fuzz    self.assertEqual(1, v)    ~~~~~~~~~~~~~~~~^^^^^^AssertionError: 1 != 194879951359646889511981556009531064497545[375 chars]50206The above exception was the direct cause of the following exception:Traceback (most recent call last):  File "D:\cpython\Lib\test\test_capi\test_long.py", line 678, in test_long_asnativebytes_fuzz    raise AssertionError(f"Value: 0x{value_hex}") from exAssertionError: Value: 0xB44EA82B9AED740C_917368BEEE924893_F6AC715721FFBD60_F1D8E1457AE009BD_473372C7566AAC3C_B12DB10F31323A43_163CEA7A98C03632_9B4C6D6C737DDAE0_1D991B33E907FAE5_B7734C3A3D49EE6B_8A01109C7FA994E7_CF15BB131B8AA2E5_6E3D4056AA61F565_9F6DD199547C4B72_1CD249CBAD015CAC_5D9AB391DEF40235_6496CC0033280BCA_DAE52FF4DE1DC081_5C43492B2921C5D2_99E4110E8C6969E5_EB220536D71B9C65_4AA79A2B1529FEtest_capi failed (1 failure)

In verbose mode:

test_long_asnativebytes_fuzz (test.test_capi.test_long.LongTests.test_long_asnativebytes_fuzz) ...52 byteshex = 1A768B041B_DFE1F8B17913408F_4A6B4BD1D6C1781B_5D3A78F4627EC370_5BFE4445BD03E6DD_8A72C2EC13593BB8_D8F338A07F90D5int = 17493562917047520716638432759788459383790970486783581286602317582080085198061893821730624261730678412225580277506146335428821FAIL

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.

encukou reacted with eyes emoji

Copy link
Member

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

@encukou
Copy link
Member

Also, please consider these changes to the docs:zooba#33

zooba reacted with thumbs up emoji

@encukou
Copy link
Member

Thank you!

zooba reacted with hooray emoji

@encukouencukou merged commit6876168 intopython:mainApr 5, 2024
@zoobazooba deleted the gh-111140 branchApril 5, 2024 14:54
diegorusso pushed a commit to diegorusso/cpython that referenced this pull requestApr 17, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@gpsheadgpsheadgpshead left review comments

@encukouencukouencukou approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@zooba@encukou@gpshead
[8]ページ先頭
©2009-2025 Movatter.jp