GeezerVison

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

GeezerVison is showing github.com — GH-96803: Add three C-API functions to make _PyInterpreterFrame less opaque for users of PEP 523. by markshannon · Pull Request #96849 · python/cpython

Conversation

markshannon commented Sep 15, 2022 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

Issue: Expose _PyInterpreterFrame_GetLine in the private API #96803

Sorry, something went wrong.

Uh oh!

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

All reactions


          Add three C-API functions to make _PyInterpreterFrame more usable for…

ce23394

… users of PEP 523.

markshannon requested a review from pablogsal

September 15, 2022 14:25

bedevere-bot added the awaiting core review label

Sep 15, 2022

pablogsal approved these changes

Sep 15, 2022

View reviewed changes

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

All reactions

Comment thread

Misc/NEWS.d/next/C API/2022-09-15-15-21-34.gh-issue-96803.ynBKIS.rst Outdated

		@@ -0,0 +1,9 @@
		Expose C-API functions to get the code object, lasti and line number from

pablogsal Sep 15, 2022

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.

Not sure if this needs a news entry since the functions are exposed technically on a private header.

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-bot added awaiting merge and removed awaiting core review labels

Sep 15, 2022


          Remove duplicate definition.

6ff2f4d

pablogsal reviewed

Sep 15, 2022

View reviewed changes

Comment thread

Include/cpython/frameobject.h Outdated


		/* Returns the currently executing line number, or -1 if there is no line number.
		* Does not raise an exception. */
		PyAPI_FUNC(int) _PyInterpreterFrame_GetLine(struct _PyInterpreterFrame *frame);

pablogsal Sep 15, 2022

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.

Using struct here is confusing the Windows compiler

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner Sep 16, 2022

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.

You can add struct _PyInterpreterFrame; somewhere to declare that "this structure exists" without having to declare the structure members (so it's declared as an opaque structure), to avoid such compiler warning. For example, at the top of this file.

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner reviewed

Sep 15, 2022

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.

Maybe added these functions to cpython/pyframe.h instead.

I don't know why frameobject.h is not included by Python.h: you have to include it explicitly, which goes against the advice of only including Python.h in C extensions. So I recently added pyframe.h which is included by Python.h. Maybe we should just move frameobject.h content into pyframe.h.

In Python 3.11, I moved multiple functions from frameobject.h to pyframe.h: 27b9894

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/frame.c Outdated

		Py_DECREF(frame->f_code);
		}

		/* Limited API functions */

vstinner Sep 15, 2022

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.

These functions are excluded from the limited API, they are declared in cpython/.

Sorry, something went wrong.

Uh oh!

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

All reactions

markshannon Sep 15, 2022

Copy link

Copy Markdown

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

What is the part of the API that isn't portable called? "unlimited", "non-limited"?

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner Sep 16, 2022

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.

The Limited API is supposed to be the most portable. The "not portable" API is called the "Public API".

For me, the reference documentation about that is now: https://devguide.python.org/developer-workflow/c-api/index.html#changing-python-s-c-api

Uh oh!

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

All reactions

Comment thread

Python/frame.c Resolved

Uh oh!

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

Comment thread

Misc/NEWS.d/next/C API/2022-09-15-15-21-34.gh-issue-96803.ynBKIS.rst Resolved

Uh oh!

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

Comment thread

Misc/NEWS.d/next/C API/2022-09-15-15-21-34.gh-issue-96803.ynBKIS.rst Resolved

Uh oh!

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

Comment thread

Include/cpython/frameobject.h Resolved

Uh oh!

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

Comment thread

Include/cpython/frameobject.h

		* implementing custom frame evaluators with PEP 523. */

		/* Returns the code object of the frame.
		* Does not raise an exception. */

vstinner Sep 15, 2022

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.

Somewhere I saw the expression: "This function cannot fail" but I can no longer find it in the doc.

Sorry, something went wrong.

Uh oh!

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

All reactions

markshannon and others added 3 commits

September 15, 2022 17:02


          Update Misc/NEWS.d/next/C API/2022-09-15-15-21-34.gh-issue-96803.ynBK…

821de76

…IS.rst

Co-authored-by: Victor Stinner <[email protected]>


          Update Include/cpython/frameobject.h

5aed2cb

Co-authored-by: Victor Stinner <[email protected]>


          Attempt to fix rst formatting.

fea57fb

smontanaro commented Nov 22, 2022

Copy link

Copy Markdown

Contributor

(I'm working my way through some PRs which have been approved and are labeled "awaiting merge", hence my seemingly bolt from the blue comment. Why? Read here.)

This PR has been idle for awhile. Can it be merged or is there more to do?

All reactions

Sorry, something went wrong.

Uh oh!

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

markshannon added 4 commits

May 4, 2023 18:45


          Make new function part of unstable API.

a6a6662


          Update news item

19bc82e


          Merge branch 'main' into py-interpreter-frame-api

53ce02c


          Fix merge.

7fbece8

iritkatriel commented May 5, 2023

Copy link

Copy Markdown

Member

Is this supposed to be in 3.12?

All reactions

Sorry, something went wrong.

Uh oh!

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

markshannon commented May 5, 2023

Copy link

Copy Markdown

Member Author

We might as well. I'm unsure how people use PEP 523, but this should help.

I'll add the Unstable prefix to them now that PEP 689 has been accepted.

All reactions

Sorry, something went wrong.

Uh oh!

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

markshannon requested a review from iritkatriel as a code owner

May 5, 2023 11:23

bedevere-bot mentioned this pull request

May 5, 2023

Expose _PyInterpreterFrame_GetLine in the private API #96803

Closed

carljm approved these changes

May 5, 2023

View reviewed changes

Comment thread

Include/cpython/frameobject.h Resolved

Uh oh!

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

markshannon force-pushed the py-interpreter-frame-api branch 2 times, most recently from 67a5450 to 7fbece8 Compare

May 5, 2023 16:23

markshannon merged commit a0df9ee into python:main

May 5, 2023

bedevere-bot removed the awaiting merge label

May 5, 2023

carljm added a commit to carljm/cpython that referenced this pull request

May 5, 2023


          Merge branch 'main' into inlinecomp2

baacf5f

* main:
  pythongh-99113: Add PyInterpreterConfig.own_gil (pythongh-104204)
  pythongh-104146: Remove unused var 'parser_body_declarations' from clinic.py (python#104214)
  pythongh-99113: Add Py_MOD_PER_INTERPRETER_GIL_SUPPORTED (pythongh-104205)
  pythongh-104108: Add the Py_mod_multiple_interpreters Module Def Slot (pythongh-104148)
  pythongh-99113: Share the GIL via PyInterpreterState.ceval.gil (pythongh-104203)
  pythonGH-100479: Add `pathlib.PurePath.with_segments()` (pythonGH-103975)
  pythongh-69152: Add _proxy_response_headers attribute to HTTPConnection (python#26152)
  pythongh-103533: Use PEP 669 APIs for cprofile (pythonGH-103534)
  pythonGH-96803: Add three C-API functions to make _PyInterpreterFrame less opaque for users of PEP 523. (pythonGH-96849)

jbower-fb pushed a commit to jbower-fb/cpython that referenced this pull request

May 8, 2023


          pythonGH-96803: Add three C-API functions to make _PyInterpreterFrame…

5405b60

… less opaque for users of PEP 523. (pythonGH-96849)

Reviewers

vstinner

vstinner left review comments

pablogsal

pablogsal approved these changes

carljm

carljm approved these changes

iritkatriel

Awaiting requested review from iritkatriel

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

7 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

markshannon commented Sep 15, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal left a comment

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!

vstinner left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smontanaro commented Nov 22, 2022

Uh oh!

iritkatriel commented May 5, 2023

Uh oh!

markshannon commented May 5, 2023

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

markshannon commented Sep 15, 2022 •

edited by bedevere-bot

Loading

Uh oh!

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