Skip to content

[docs] Mention how to get/set a bigint PyLong via the C API #101270

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

Merged
merged 5 commits into from
Jan 24, 2023

Conversation

gpshead
Copy link
Member

@gpshead gpshead commented Jan 23, 2023

There isn't enough demand for a direct C API to get or set a binary byte array bigint representation of PyLong but we do want the people who need to understand how they can do that.

Background: https://discuss.python.org/t/add-portable-way-to-extract-all-digits-of-a-pylongobject-from-c/20045

We don't need direct C APIs to get at a bigint representation of PyLong but we
do want the few people who need to understand how.
@gpshead gpshead requested a review from CAM-Gerlach January 23, 2023 23:02
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Two high-level comments:

  • Given how this is rendered, as a small normal prose paragraph kinda buried at the end of a long list of APIs, it seems rather hard to find and easy to miss. I would suggest using the standard .. see-also:: directive to style it appropriately, and move it to the end of the top-level description on line 15.

  • Additionally, until I looked at the linked thread, I didn't immediately grasp the actual significance of this from the wording here—that int.to_bytes is the (only supported) way to get the full bigint value of a PyLongObject, and int.from_bytes the (only supported) way to roundtrip an arbitrarily-long bigint value back into a PyLongObject. Rather, at least to me (as a decidedly non-expert), it just sounded like a way to convert a PyLongObject to/from one particular representation of many. Therefore, I would suggest explicitly mentioning this for the reader.

To address both of those, I suggest moving this to line 15, with something like the following text (I can't make it as a GitHub suggestion, though I could commit it to your branch, if desired):

.. seealso:: Python methods :meth:`int.to_bytes` and :meth:`int.from_bytes`
   (called via one of the :c:func:`PyObject_CallMethod` C APIs),
   to convert a :c:type:`PyLongObject` of arbitrary size
   to or from an array of bytes.

@CAM-Gerlach
Copy link
Member

Also, is there a particular reason not to backport this to 3.10 as well? As far as I can tell, none of the referenced APIs have changed significantly since that version, and general docs policy (AFAIK) is to backport docs changes that aren't specific to any version to all bugfix-supported versions, to maximize benefit and minimize merge conflicts for future backports.

@gpshead gpshead added the needs backport to 3.10 only security fixes label Jan 24, 2023
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small suggested tweak to my previous wording, otherwise LGTM. Thanks @gpshead !

@gpshead
Copy link
Member Author

gpshead commented Jan 24, 2023

thanks, exactly the kind of docs feedback I needed. I'm trying it out moved down next to PyLong_FromString as that is the API it most resembles. With an explicit note of to/from bytes meaning base 256. the seealso at top looked a little too prominent and disconnected from other things.

@gpshead gpshead merged commit e244401 into python:main Jan 24, 2023
@miss-islington
Copy link
Contributor

Thanks @gpshead for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@gpshead gpshead deleted the docs/PyLong-C-bigint-howto branch January 24, 2023 05:20
@bedevere-bot
Copy link

GH-101275 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jan 24, 2023
@bedevere-bot
Copy link

GH-101276 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jan 24, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 24, 2023
…-101270)

We don't need direct C APIs to get at a bigint representation of PyLong but we
do want the few people who need to understand how.

Additional Author:  CAM-Gerlach
(cherry picked from commit e244401)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 24, 2023
…-101270)

We don't need direct C APIs to get at a bigint representation of PyLong but we
do want the few people who need to understand how.

Additional Author:  CAM-Gerlach
(cherry picked from commit e244401)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
miss-islington added a commit that referenced this pull request Jan 24, 2023
We don't need direct C APIs to get at a bigint representation of PyLong but we
do want the few people who need to understand how.

Additional Author:  CAM-Gerlach
(cherry picked from commit e244401)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
miss-islington added a commit that referenced this pull request Jan 24, 2023
We don't need direct C APIs to get at a bigint representation of PyLong but we
do want the few people who need to understand how.

Additional Author:  CAM-Gerlach
(cherry picked from commit e244401)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
@CAM-Gerlach
Copy link
Member

I'm trying it out moved down next to PyLong_FromString as that is the API it most resembles.

I put some thought toward proposing splitting the SeeAlso to having the from one under FromString and the to one under the most relevant to As function(s), perhaps PyLong_AsLongLongAndOverflow, to not draw too much attention to them while making them most proximate to the places users would likely go looking. I ended up not doing so as I was concerned users just skimming the functions for any that were relevant might not notice it, and there wasn't a single obvious candidate for the As/to side of it.

It's certainly a valid approach nonetheless, but the problem with the current implementation is that both the to and from methods are both listed under a from function, and nothing is listed under and of the As functions, thus users looking in that direction may never find it. This would seem to neglect the most probable and necessary use case, the one highlighted in the thread—needing to get the value of a bigint bigger than a long long—to which the current exposed APIs don't offer anything at all comparable (unlike FromString in the other direction, as you mention).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip issue skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants