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: Improve PyLong_AsNativeBytes API doc example & improve the test by gpshead · Pull Request #115380 · python/cpython

Conversation

@gpshead

@gpshead gpshead commented Feb 12, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

This expands the examples to cover both realistic use cases for the API.

I noticed thing in the test that could be done better so I added those as well: We need to guarantee that all bytes of the result are overwritten and that too many are not written. Tests now pre-fills the result with data in order to ensure that. (assuming ctypes isn't undoing that behind the scenes...)

📚 Documentation preview 📚: https://cpython-previews--115380.org.readthedocs.build/

  • 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
gpshead added 2 commits February 12, 2024 14:46
@gpshead
Fix the API name in the doc example, improve test.
0302840 
This also makes the example more realistic, people are unlikely to want
this API to get a value into a simple `int` type, we've got direct APIs
for that. So have the example use an array as if it is a bignum of its
own.

I start by fixing the API name in the doc example as scoder@ noted in
the original code review after merge.  But I noticed one thing in the
test that could be done better so I added that as well: we need to
guarantee that all bytes of the result are overwritten.  This now
pre-fills the result with data in order to ensure that.  _(assuming
ctypes isn't undoing that behind the scenes...)_
@gpshead
further test refinements.
5099616
@gpshead gpshead added skip news 3.13 only security fixes labels Feb 12, 2024
@gpshead gpshead self-assigned this Feb 12, 2024
@bedevere-app bedevere-app Bot added the tests Tests in the Lib/test dir label Feb 12, 2024
gpshead added 2 commits February 12, 2024 15:12
@gpshead
Merge branch 'main' into fixup/pythongh-11140
4859a9b
@gpshead
One more assertion against n=0 writes.
97292ba
@gpshead
gpshead requested a review from zooba February 12, 2024 23:16
@gpshead
gpshead marked this pull request as ready for review February 12, 2024 23:16
@bedevere-app bedevere-app Bot added the awaiting core review label Feb 12, 2024
@gpshead gpshead changed the title gh-11140: Correct AsNativeBytes API docs & improve the test gh-11140: Improve PyLong_AsNativeBytes API doc example & improve the test Feb 12, 2024
@gpshead gpshead mentioned this pull request Feb 12, 2024
gh-111140: Adds PyLong_AsNativeBytes and PyLong_FromNative[Unsigned]Bytes functions #114886
Merged
@zooba

zooba commented Feb 13, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

I disagree with the example change - the intent is certainly to copy directly into simple types, but the difference is that the size of the type is explicit (once things have stabilised a bit I intend to try replacing the implementations of our various typed ones with calls to this function and benchmark).

Perhaps using long would be better, as that more obviously varies between platforms (reliably 32-bit on Windows; pointer-sized on POSIX)?

But this is absolutely a way to provide a single reliable function to replace all the others.

The edge case doesn't come up in the int example because int is signed, and so you would get the failure you expect if the sign bit doesn't fit. An unsigned int example would potentially trigger it, as values in [0x80000000, 0xFFFFFFFF] would fit, but this API will return 5 and so it looks like a failure. But for int, it does exactly the right thing for all values.

The additional test changes are good though. I like those
gpshead 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.
@zooba

zooba commented Feb 13, 2024

Copy link
Copy Markdown
Member

Alternatively, maybe use int32_t for the example?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zooba zooba changed the title gh-11140: Improve PyLong_AsNativeBytes API doc example & improve the test gh-111140: Improve PyLong_AsNativeBytes API doc example & improve the test Feb 13, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Feb 13, 2024
C API: Consider adding public PyLong_AsByteArray() and PyLong_FromByteArray() functions #111140
Closed
@gpshead
gpshead marked this pull request as draft February 14, 2024 20:39
@bedevere-app bedevere-app Bot removed the awaiting core review label Feb 14, 2024
@gpshead
Clarify, document both use cases.
71d527f
@gpshead

gpshead commented Feb 17, 2024

Copy link
Copy Markdown
Member Author

Take another look, I updated it to cover both use cases and try to be more specific.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zooba
zooba reviewed Feb 19, 2024
View reviewed changes
Comment thread Doc/c-api/long.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/c-api/long.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/c-api/long.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/c-api/long.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/c-api/long.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_capi/test_long.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
gpshead and others added 3 commits February 20, 2024 16:51
@gpshead
A little more verbose to avoid PyErr_Occurred.
b04aa97 
We don't want examples to encourage PyErr_Occurred on modern APIs.
@gpshead @zooba
Update Doc/c-api/long.rst
cf1dd9c 
Co-authored-by: Steve Dower <[email protected]>
@gpshead
remove redundant text that bloats the note.
ed5b887
@gpshead
gpshead marked this pull request as ready for review February 21, 2024 01:04
@bedevere-app bedevere-app Bot added the awaiting core review label Feb 21, 2024
zooba
zooba reviewed Feb 21, 2024
View reviewed changes

@zooba zooba 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.

LGTM with the change noted (I don't mind which way you go with it)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Doc/c-api/long.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@gpshead
Switch the sentence in the note.
937df33
@gpshead
gpshead enabled auto-merge (squash) February 22, 2024 03:07
@gpshead
gpshead merged commit fac99b8 into python:main Feb 22, 2024
@bedevere-app bedevere-app Bot removed the awaiting core review label Feb 22, 2024
@gpshead
gpshead deleted the fixup/gh-11140 branch February 22, 2024 19:12
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
@gpshead @zooba @woodruffw
pythongh-111140: Improve PyLong_AsNativeBytes API doc example & impro…
54fae23 
…ve the test (python#115380)

This expands the examples to cover both realistic use cases for the API.
    
I noticed thing in the test that could be done better so I added those as well: We need to guarantee that all bytes of the result are overwritten and that too many are not written.  Tests now pre-fills the result with data in order to ensure that.

Co-authored-by: Steve Dower <[email protected]>
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@gpshead @zooba @diegorusso
pythongh-111140: Improve PyLong_AsNativeBytes API doc example & impro…
d21b2d6 
…ve the test (python#115380)

This expands the examples to cover both realistic use cases for the API.
    
I noticed thing in the test that could be done better so I added those as well: We need to guarantee that all bytes of the result are overwritten and that too many are not written.  Tests now pre-fills the result with data in order to ensure that.

Co-authored-by: Steve Dower <[email protected]>
LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request Jan 22, 2025
@gpshead @zooba @LukasWoodtli
pythongh-111140: Improve PyLong_AsNativeBytes API doc example & impro…
b1b27e6 
…ve the test (python#115380)

This expands the examples to cover both realistic use cases for the API.
    
I noticed thing in the test that could be done better so I added those as well: We need to guarantee that all bytes of the result are overwritten and that too many are not written.  Tests now pre-fills the result with data in order to ensure that.

Co-authored-by: Steve Dower <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zooba zooba zooba left review comments

Uh oh!

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

Assignees

@gpshead gpshead

Labels

3.13 only security fixes skip news tests Tests in the Lib/test dir

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.

2 participants

@gpshead @zooba
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.