Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Conversation
gpshead commented Feb 12, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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...)_
zooba commented Feb 13, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 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 The additional test changes are good though. I like those |
zooba commented Feb 13, 2024
Alternatively, maybe use |
gpshead commented Feb 17, 2024
Take another look, I updated it to cover both use cases and try to be more specific. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
We don't want examples to encourage PyErr_Occurred on modern APIs.
Co-authored-by: Steve Dower <steve.dower@microsoft.com>
zooba left a comment
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)
Uh oh!
There was an error while loading. Please reload this page.
…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>
…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>
…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>
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/