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: Improve PyLong_AsNativeBytes API doc example & improve the test#115380

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
gpshead merged 9 commits intopython:mainfromgpshead:fixup/gh-11140
Feb 22, 2024

Conversation

gpshead
Copy link
Member

@gpsheadgpshead commentedFeb 12, 2024•
edited
Loading

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/

This also makes the example more realistic, people are unlikely to wantthis API to get a value into a simple `int` type, we've got direct APIsfor that. So have the example use an array as if it is a bignum of itsown.I start by fixing the API name in the doc example as scoder@ noted inthe original code review after merge.  But I noticed one thing in thetest that could be done better so I added that as well: we need toguarantee that all bytes of the result are overwritten.  This nowpre-fills the result with data in order to ensure that.  _(assumingctypes isn't undoing that behind the scenes...)_
@gpsheadgpshead added skip news 3.13bugs and security fixes labelsFeb 12, 2024
@gpsheadgpshead self-assigned thisFeb 12, 2024
@bedevere-appbedevere-appbot added the testsTests in the Lib/test dir labelFeb 12, 2024
@gpsheadgpshead requested a review fromzoobaFebruary 12, 2024 23:16
@gpsheadgpshead marked this pull request as ready for reviewFebruary 12, 2024 23:16
@gpsheadgpshead changed the titlegh-11140: Correct AsNativeBytes API docs & improve the testgh-11140: Improve PyLong_AsNativeBytes API doc example & improve the testFeb 12, 2024
@zooba
Copy link
Member

zooba commentedFeb 13, 2024•
edited
Loading

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 usinglong 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 theint example becauseint is signed, and so you would get the failure you expect if the sign bit doesn't fit. Anunsigned int example would potentially trigger it, as values in[0x80000000, 0xFFFFFFFF] would fit, but this API will return5 and so it looks like a failure. But forint, 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

@zooba
Copy link
Member

Alternatively, maybe useint32_t for the example?

@zoobazooba changed the titlegh-11140: Improve PyLong_AsNativeBytes API doc example & improve the testgh-111140: Improve PyLong_AsNativeBytes API doc example & improve the testFeb 13, 2024
@gpsheadgpshead marked this pull request as draftFebruary 14, 2024 20:39
@gpshead
Copy link
MemberAuthor

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

gpsheadand others added3 commitsFebruary 20, 2024 16:51
We don't want examples to encourage PyErr_Occurred on modern APIs.
Co-authored-by: Steve Dower <steve.dower@microsoft.com>
@gpsheadgpshead marked this pull request as ready for reviewFebruary 21, 2024 01:04
Copy link
Member

@zoobazooba left a 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)

gpshead reacted with thumbs up emoji
@gpsheadgpsheadenabled auto-merge (squash)February 22, 2024 03:07
@gpsheadgpshead merged commitfac99b8 intopython:mainFeb 22, 2024
@gpsheadgpshead deleted the fixup/gh-11140 branchFebruary 22, 2024 19:12
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull requestMar 4, 2024
…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 <steve.dower@microsoft.com>
diegorusso pushed a commit to diegorusso/cpython that referenced this pull requestApr 17, 2024
…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 <steve.dower@microsoft.com>
LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull requestJan 22, 2025
…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 <steve.dower@microsoft.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@zoobazoobazooba left review comments

Assignees

@gpsheadgpshead

Labels
3.13bugs and security fixesskip newstestsTests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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