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

Merged
merged 9 commits into from
Feb 22, 2024

Conversation

gpshead
Copy link
Member

@gpshead gpshead commented Feb 12, 2024

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 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 gpshead added skip news 3.13 bugs and 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 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
@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
@zooba
Copy link
Member

zooba commented Feb 13, 2024

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

@zooba
Copy link
Member

zooba commented Feb 13, 2024

Alternatively, maybe use int32_t for the example?

@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
@gpshead gpshead marked this pull request as draft February 14, 2024 20:39
@gpshead
Copy link
Member Author

gpshead commented Feb 17, 2024

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

Doc/c-api/long.rst Outdated Show resolved Hide resolved
Doc/c-api/long.rst Show resolved Hide resolved
Doc/c-api/long.rst Show resolved Hide resolved
Doc/c-api/long.rst Outdated Show resolved Hide resolved
Doc/c-api/long.rst Outdated Show resolved Hide resolved
Lib/test/test_capi/test_long.py Show resolved Hide resolved
gpshead and others added 3 commits February 20, 2024 16:51
We don't want examples to encourage PyErr_Occurred on modern APIs.
Co-authored-by: Steve Dower <[email protected]>
@gpshead gpshead marked this pull request as ready for review February 21, 2024 01:04
Copy link
Member

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

Doc/c-api/long.rst Show resolved Hide resolved
@gpshead gpshead enabled auto-merge (squash) February 22, 2024 03:07
@gpshead gpshead merged commit fac99b8 into python:main Feb 22, 2024
32 checks passed
@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
…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
…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
Labels
3.13 bugs and security fixes skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants