GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-115653: Add docs for the PyCode_GetFirstFree and correct return type for the PyCode_GetNumFree by wrongnull · Pull Request #115654 · python/cpython

Conversation

wrongnull commented Feb 19, 2024 •

edited by github-actions Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Issue: Missing documentation for public C-API function #115653

📚 Documentation preview 📚: https://cpython-previews--115654.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

andiserg reacted with thumbs up emoji

All reactions

👍 1 reaction


          Add docs for public c-api function

bedevere-app Bot added awaiting review docs

Documentation in the Doc dir

skip news labels

Feb 19, 2024

bedevere-app Bot mentioned this pull request

Feb 19, 2024

Missing documentation for public C-API function #115653

Closed


          add missed period

56f4c39

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 added the needs backport to 3.12 only security fixes label

Feb 19, 2024

Eclips4 reviewed

Feb 19, 2024

View reviewed changes

Comment thread

Doc/c-api/code.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Eclips4 requested a review from vstinner

February 20, 2024 08:14

wrongnull commented Feb 20, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reviewed

Feb 20, 2024

View reviewed changes

Comment thread

Doc/c-api/code.rst Outdated


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

		Return the number of local + cell variables in co.

erlend-aasland Feb 20, 2024

Copy link

Copy Markdown

Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

wrongnull Feb 20, 2024

Copy link

Copy Markdown

Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

erlend-aasland Feb 20, 2024

Copy link

Copy Markdown

Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

wrongnull Feb 20, 2024

Copy link

Copy Markdown

Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with thumbs up emoji

All reactions

👍 1 reaction

erlend-aasland Feb 21, 2024

Copy link

Copy Markdown

Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

wrongnull reacted with eyes emoji

All reactions

👀 1 reaction

markshannon Feb 21, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland and wrongnull reacted with thumbs up emoji

All reactions

👍 2 reactions

erlend-aasland Feb 21, 2024

Copy link

Copy Markdown

Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

erlend-aasland Feb 21, 2024

Copy link

Copy Markdown

Contributor

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.

Follow-up issue: #115756.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Change the wording of the function documentation

6619bb9

erlend-aasland commented Feb 21, 2024

Copy link

Copy Markdown

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.

wrongnull reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner reviewed

Feb 21, 2024

View reviewed changes

Comment thread

Doc/c-api/code.rst Outdated

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

		Return the number of free variables in co.

vstinner Feb 21, 2024

Copy link

Copy Markdown

Member

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.

Suggested change

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Doc/c-api/code.rst Outdated


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

		Return the position of first free variable in co.

vstinner Feb 21, 2024

Copy link

Copy Markdown

Member

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.

Suggested change

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

wrongnull Feb 21, 2024

Copy link

Copy Markdown

Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Eleminate abbreviation and add article

2af8bee

vstinner approved these changes

Feb 21, 2024

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

wrongnull reacted with heart emoji

All reactions

❤️ 1 reaction

bedevere-app Bot added awaiting merge and removed awaiting review labels

Feb 21, 2024

vstinner merged commit 10fc467 into python:main

Feb 21, 2024

bedevere-app Bot removed the awaiting merge label

Feb 21, 2024

miss-islington-app Bot commented Feb 21, 2024

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Feb 21, 2024


          pythongh-115653: Document PyCode_GetFirstFree() (pythonGH-115654)

a75d804

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

Co-authored-by: Bogdan Romanyuk <[email protected]>

bedevere-app Bot commented Feb 21, 2024

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot removed the needs backport to 3.12 only security fixes label

Feb 21, 2024

erlend-aasland reviewed

Feb 21, 2024

View reviewed changes

Comment thread

Doc/c-api/code.rst

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

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

erlend-aasland Feb 21, 2024

Copy link

Copy Markdown

Contributor

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.

@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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Feb 21, 2024

Copy link

Copy Markdown

Member

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.

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 :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

erlend-aasland Feb 21, 2024

Copy link

Copy Markdown

Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Eclips4 reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Doc/c-api/code.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland mentioned this pull request

Feb 21, 2024

Make PyCode_GetFirstFree a PEP-689 unstable API #115756

Closed

wrongnull deleted the patch-3 branch

February 21, 2024 10:30

vstinner pushed a commit that referenced this pull request

Feb 21, 2024


          [3.12] gh-115653: Document PyCode_GetFirstFree() (GH-115654) (#115752)

8d0ec39

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 <[email protected]>

wrongnull mentioned this pull request

Feb 21, 2024

gh-115756: make PyCode_GetFirstFree an unstable API #115781

Merged

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request

Mar 4, 2024


          pythongh-115653: Document PyCode_GetFirstFree() (python#115654)

0ff9c05

Correct the return type of the PyCode_GetNumFree() documentation.

encukou mentioned this pull request

Mar 8, 2024

Make PyCode_GetFirstFree unstable capi-workgroup/decisions#18

Closed

5 tasks done

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request

Apr 17, 2024


          pythongh-115653: Document PyCode_GetFirstFree() (python#115654)

3df8eac

Correct the return type of the PyCode_GetNumFree() documentation.

LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request

Jan 22, 2025


          pythongh-115653: Document PyCode_GetFirstFree() (python#115654)

aeced76

Correct the return type of the PyCode_GetNumFree() documentation.

Reviewers

vstinner

vstinner approved these changes

markshannon

markshannon left review comments

erlend-aasland

erlend-aasland left review comments

Eclips4

Eclips4 left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

docs

Documentation in the Doc dir

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

wrongnull commented Feb 19, 2024 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

wrongnull commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markshannon Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erlend-aasland commented Feb 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington-app Bot commented Feb 21, 2024

Uh oh!

bedevere-app Bot commented Feb 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wrongnull commented Feb 19, 2024 •

edited by github-actions Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

wrongnull commented Feb 20, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

markshannon Feb 21, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.