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-115653: Add docs for the PyCode_GetFirstFree and correct return type for the PyCode_GetNumFree #115654

Merged
merged 4 commits into from
Feb 21, 2024

Conversation

wrongnull
Copy link
Contributor

@wrongnull wrongnull commented Feb 19, 2024

@wrongnull wrongnull changed the title gh-115653: Add docs for the PyCode_GetFirstFree and correct return type for the PyCode_GetFirstFree gh-115653: Add docs for the PyCode_GetFirstFree and correct return type for the PyCode_GetNumFree Feb 19, 2024
@Eclips4 Eclips4 added the needs backport to 3.12 bug and security fixes label Feb 19, 2024
@wrongnull
Copy link
Contributor Author

wrongnull commented Feb 20, 2024

BTW, should PyCode_GetFirstFree cast the return value to Py_ssize_t? As I can see this function is used mostly for calculating offsets.


Return the number of free variables in *co*.

.. c:function:: int PyCode_GetFirstFree(PyCodeObject *co)

Return the number of local + cell variables in *co*.
Copy link
Contributor

Choose a reason for hiding this comment

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

Document what the API does, not its implementation details: PyCode_GetFirstFree returns the position (or offset) of the first free variable in co.

FTR: @markshannon and @iritkatriel introduced this API in gh-100721. It is available via Python.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. But my question above still stands. If I'm right I might make a separate pull request for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

See the PR I linked to, especially #100721 (comment) and 49ca044.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. If there is anything else here that I could fix, I'll be happy to do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would still like to hear from either @markshannon or @iritkatriel; I'm not sure this was intended as a public API.

Copy link
Member

@markshannon markshannon Feb 21, 2024

Choose a reason for hiding this comment

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

It was and it wasn't. We don't really want to expose it, but if we don't then Cython and other C extensions will access the internal fields directly, which is worse.

Now that we have an unstable API, it should probably be part of that.
I'd like all code object C APIs to be unstable, but it's probably too late for that.

Maybe rename it to PyUnstableCode_GetFirstFree?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rename it to PyUnstableCode_GetFirstFree?

Sounds like a good idea, but since it is already included in 3.12, we have to deprecate PyCode_GetFirstFree and PyUnstableCode_GetFirstFree in 3.13.

Copy link
Contributor

Choose a reason for hiding this comment

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

Follow-up issue: #115756.

@erlend-aasland
Copy link
Contributor

In general, please split up multiple changes in atomic PRs; the distinct changes might not have equal backport requirements. For these two changes it looks to me both of them are applicable for a 3.12 backport only, so you're fine.

@@ -30,10 +30,14 @@ bound into a function.
Return true if *co* is a :ref:`code object <code-objects>`.
This function always succeeds.

.. c:function:: int PyCode_GetNumFree(PyCodeObject *co)
.. c:function:: Py_ssize_t PyCode_GetNumFree(PyCodeObject *co)

Return the number of free variables in *co*.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Return the number of free variables in *co*.
Return the number of free variables in a code object.


Return the number of free variables in *co*.

.. c:function:: int PyCode_GetFirstFree(PyCodeObject *co)

Return the position of first free variable in *co*.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Return the position of first free variable in *co*.
Return the position of the first free variable in a code object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@vstinner vstinner merged commit 10fc467 into python:main Feb 21, 2024
25 checks passed
@miss-islington-app
Copy link

Thanks @wrongnull for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 21, 2024
Correct the return type of the PyCode_GetNumFree() documentation.
(cherry picked from commit 10fc467)

Co-authored-by: Bogdan Romanyuk <65823030+wrongnull@users.noreply.github.com>
@bedevere-app
Copy link

bedevere-app bot commented Feb 21, 2024

GH-115752 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Feb 21, 2024

Return the number of free variables in *co*.
Return the number of free variables in a code object.
Copy link
Contributor

Choose a reason for hiding this comment

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

@vstinner, I think this is the suggested style:

Suggested change
Return the number of free variables in a code object.
Return the number of free variables in code object *co*.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I just merged the change. Is it worth it to change the style, or is it ok to merge PR gh-115752 backport? I let you decide :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's not worth it to change the style, but I think we should try to keep it in mind for next time ;)

Doc/c-api/code.rst Show resolved Hide resolved
@wrongnull wrongnull deleted the patch-3 branch February 21, 2024 10:30
vstinner pushed a commit that referenced this pull request Feb 21, 2024
gh-115653: Document PyCode_GetFirstFree() (GH-115654)

Correct the return type of the PyCode_GetNumFree() documentation.
(cherry picked from commit 10fc467)

Co-authored-by: Bogdan Romanyuk <65823030+wrongnull@users.noreply.github.com>
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
Correct the return type of the PyCode_GetNumFree() documentation.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
Correct the return type of the PyCode_GetNumFree() documentation.
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 news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants