GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-111140: Fix edge case in PyLong_AsNativeBytes where large negative longs may require an extra byte by zooba · Pull Request #116053 · python/cpython

Conversation

@zooba

@zooba zooba commented Feb 28, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: C API: Consider adding public PyLong_AsByteArray() and PyLong_FromByteArray() functions #111140

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@zooba
pythongh-111140: Fix edge case in PyLong_AsNativeBytes where large ne…
d5b0e98 
…gative longs may require an extra byte
@zooba
zooba marked this pull request as ready for review February 28, 2024 15:58
@bedevere-app bedevere-app Bot mentioned this pull request Feb 28, 2024
C API: Consider adding public PyLong_AsByteArray() and PyLong_FromByteArray() functions #111140
Closed
@bedevere-app bedevere-app Bot added the awaiting core review label Feb 28, 2024
zooba
zooba commented Feb 28, 2024
View reviewed changes
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@zooba
zooba requested a review from gpshead February 28, 2024 16:22
gpshead
gpshead reviewed Feb 28, 2024
View reviewed changes
Comment thread Lib/test/test_capi/test_long.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@gpshead
gpshead marked this pull request as draft February 28, 2024 23:55
@bedevere-app bedevere-app Bot removed the awaiting core review label Feb 28, 2024
@zooba
Improved edge case testing and fuzz tests
f162348
@zooba

zooba commented Mar 1, 2024

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zooba added 3 commits March 1, 2024 00:50
@zooba
Test comments
0efa143
@zooba
Fix for x86
15b3752
@zooba
Merge remote-tracking branch 'upstream/main' into pythongh-111140
fbe42e7
@zooba
zooba marked this pull request as ready for review March 19, 2024 22:14
@bedevere-app bedevere-app Bot added the awaiting core review label Mar 19, 2024
@zooba
More precise check for unsigned non-overflow
8dedad7
@zooba zooba mentioned this pull request Mar 28, 2024
Support more integer types in PyMemberDef #117031
Open
zooba added 2 commits March 28, 2024 17:19
@zooba
Merge branch 'main' into pythongh-111140
d3deebf
@zooba
Change endianness arguments to flags to allow additional arguments
621708c
@zooba zooba added the DO-NOT-MERGE label Mar 28, 2024
@zooba

zooba commented Mar 28, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Updated the endianness argument to flags 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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zooba added 3 commits March 28, 2024 18:14
@zooba
Space
c607f04
@zooba
Fix tests, one result, and docs
140c011
@zooba
Fix test
77097bf
@zooba zooba removed the DO-NOT-MERGE label Mar 28, 2024
zooba added 2 commits March 28, 2024 21:49
@zooba
Update NEWS
a8e2540
@zooba
Missed colon
5ba6451
encukou
encukou reviewed Mar 29, 2024
View reviewed changes

@encukou encukou left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zooba reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/test/test_capi/test_long.py
*[rng.randrange(256) for _ in range(n - 1)]
])
bytes_le = bytes_be[::-1]
v = int.from_bytes(bytes_le, 'little')

@encukou encukou Mar 29, 2024

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@zooba zooba Apr 2, 2024

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
encukou reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@zooba
Doc fixes and improved test failure output
68b9d8e
@zooba
Remove my deliberate failure
37ad671
@zooba

zooba commented Apr 2, 2024

Copy link
Copy Markdown
Member Author

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

0:00:00 Run 1 test sequentially
0:00:00 [1/1] test_capi
test 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]50206

The 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 ex
AssertionError: Value: 0xB44EA82B9AED740C_917368BEEE924893_F6AC715721FFBD60_F1D8E1457AE009BD_473372C7566AAC3C_B12DB10F31323A43_163CEA7A98C03632_9B4C6D6C737DDAE0_1D991B33E907FAE5_B7734C3A3D49EE6B_8A01109C7FA994E7_CF15BB131B8AA2E5_6E3D4056AA61F565_9F6DD199547C4B72_1CD249CBAD015CAC_5D9AB391DEF40235_6496CC0033280BCA_DAE52FF4DE1DC081_5C43492B2921C5D2_99E4110E8C6969E5_EB220536D71B9C65_4AA79A2B1529FE

test_capi failed (1 failure)

In verbose mode:

test_long_asnativebytes_fuzz (test.test_capi.test_long.LongTests.test_long_asnativebytes_fuzz) ...
52 bytes
hex = 1A768B041B_DFE1F8B17913408F_4A6B4BD1D6C1781B_5D3A78F4627EC370_5BFE4445BD03E6DD_8A72C2EC13593BB8_D8F338A07F90D5
int = 17493562917047520716638432759788459383790970486783581286602317582080085198061893821730624261730678412225580277506146335428821
FAIL

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
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
encukou
encukou reviewed Apr 4, 2024
View reviewed changes

@encukou encukou left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@encukou

encukou commented Apr 4, 2024

Copy link
Copy Markdown
Member

Also, please consider these changes to the docs: zooba#33
zooba reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
encukou and others added 4 commits April 4, 2024 15:35
@encukou
Reword PyLong_AsNativeBytes docs (#33)
1e615cf
@zooba
Apply PR feedback
d1b886c
@zooba
Merge branch 'pythongh-111140' of https://github.com/zooba/cpython into
38ecfe2 
pythongh-111140
@zooba
Check for new error
614a6ae
encukou
encukou approved these changes Apr 5, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Apr 5, 2024
@encukou

encukou commented Apr 5, 2024

Copy link
Copy Markdown
Member

Thank you!
zooba reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@encukou
encukou merged commit 6876168 into python:main Apr 5, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Apr 5, 2024
@zooba
zooba deleted the gh-111140 branch April 5, 2024 14:54
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@zooba @diegorusso
pythongh-111140: PyLong_From/AsNativeBytes: Take *flags* rather than …
29db213 
…just *endianness* (pythonGH-116053)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead left review comments
@encukou encukou encukou approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@zooba @encukou @gpshead
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.