Skip to content

gh-120834: fix type of *_iframe field in _PyGenObject_HEAD declaration#120835

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.

Sign up for GitHub

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

Jump to bottom
Merged
iritkatriel merged 6 commits into from
Jun 24, 2024
Merged

gh-120834: fix type of *_iframe field in _PyGenObject_HEAD declaration #120835

iritkatriel merged 6 commits into from
Jun 24, 2024

Conversation

@iritkatriel
Copy link
Member

@iritkatrieliritkatriel commented Jun 21, 2024•
edited by bedevere-app bot
Loading

@bedevere-appbedevere-appbot mentioned this pull request Jun 21, 2024
Closed
iritkatriel
iritkatriel commented Jun 21, 2024
View reviewed changes
@iritkatrieliritkatriel added skip news interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jun 21, 2024
mdboom
mdboom approved these changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@mdboommdboom left a comment

Choose a reason for hiding this comment

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

LGTM

markshannon
markshannon requested changes Jun 21, 2024
View reviewed changes
Copy link
Member

@markshannonmarkshannon left a comment

Choose a reason for hiding this comment

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

To do this we'll need to either:
Move _PyInterpreterFrame to a public header
Move _PyGenObject_HEAD to a private header.

If you can avoid breaking the API, the second option is preferable.

@bedevere-app
Copy link

bedevere-appbot commented Jun 21, 2024

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@iritkatriel
Copy link
MemberAuthor

iritkatriel commented Jun 21, 2024

To do this we'll need to either: Move _PyInterpreterFrame to a public header Move _PyGenObject_HEAD to a private header.

If you can avoid breaking the API, the second option is preferable.

PyGenObject is API, so I don't think the second option is possible.

@markshannon
Copy link
Member

markshannon commented Jun 21, 2024•
edited
Loading

Is the actual structure of PyGenObject part of the API, or just its existence?
All the API functions take PyGenObject *, so we can move the structure definition to an internal header and leave
typedef struct _PyGenObject PyGenObject; in the public header.

iritkatriel
iritkatriel commented Jun 21, 2024
View reviewed changes
@iritkatriel
Copy link
MemberAuthor

iritkatriel commented Jun 21, 2024

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-appbot commented Jun 21, 2024

Thanks for making the requested changes!

@mdboom, @markshannon: please review the changes made to this pull request.

markshannon
markshannon approved these changes Jun 22, 2024
View reviewed changes
Copy link
Member

@markshannonmarkshannon left a comment

Choose a reason for hiding this comment

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

Nice, this will definitely make things more maintainable.

It would be good to get rid of the _PyGenObject_HEAD macro, but that's for another PR.

@iritkatrieliritkatriel added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 23, 2024
@bedevere-bot
Copy link

bedevere-bot commented Jun 23, 2024

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit c9ed63f 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 23, 2024
@iritkatriel
Copy link
MemberAuthor

iritkatriel commented Jun 23, 2024

It would be good to get rid of the _PyGenObject_HEAD macro, but that's for another PR.

Do you know the history, why we have three structs with the same structure but different names?

@markshannon
Copy link
Member

markshannon commented Jun 24, 2024

It would be good to get rid of the _PyGenObject_HEAD macro, but that's for another PR.

Do you know the history, why we have three structs with the same structure but different names?

Probably to avoid copy and pasting the struct for the coroutine classes and keep PyGenObject unchanged.

@iritkatrieliritkatriel merged commit 65a12c5 into python:mainJun 24, 2024
markshannon
markshannon reviewed Jun 24, 2024
View reviewed changes
Objects/genobject.c
"generator", /* tp_name */
offsetof(PyGenObject, gi_iframe) +
offsetof(_PyInterpreterFrame, localsplus), /* tp_basicsize */
sizeof(PyGenObject), /* tp_basicsize */
Copy link
Member

@markshannonmarkshannonJun 24, 2024

Choose a reason for hiding this comment

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

I think this should be offsetof(PyGenObject, gi_frame.localsplus) otherwise we are overallocating one PyObject * slot.

Likewise for PyCoroObject and PyAsyncGenObject

Copy link
MemberAuthor

@iritkatrieliritkatrielJun 24, 2024

Choose a reason for hiding this comment

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

Let's try that: #120941

@iritkatriel
Copy link
MemberAuthor

iritkatriel commented Jun 24, 2024

Probably to avoid copy and pasting the struct for the coroutine classes and keep PyGenObject unchanged.

But PyGenObject , PyCoroObject and PyAsyncGenObject are the same thing. Why don't we have just one type for all of them?

@iritkatrieliritkatriel mentioned this pull request Jun 24, 2024
Merged
@markshannon
Copy link
Member

markshannon commented Jun 24, 2024

They haven't always been the same 299483c

mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
@iritkatriel@mrahtz
pythongh-120834: fix type of *_iframe field in _PyGenObject_HEAD decl…
818e5ed
…aration (python#120835)
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
@iritkatriel@noahbkim
pythongh-120834: fix type of *_iframe field in _PyGenObject_HEAD decl…
250d607
…aration (python#120835)
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@iritkatriel@estyxx
pythongh-120834: fix type of *_iframe field in _PyGenObject_HEAD decl…
d419f30
…aration (python#120835)
@cdce8pcdce8p mentioned this pull request Oct 13, 2024
Merged
JukkaL pushed a commit to python/mypy that referenced this pull request Oct 14, 2024
@cdce8p
[mypyc] Use PyGen_GetCode in gen_is_coroutine (#17931)
e6ced48
Instead of copying the implementation of `_PyGen_GetCode` every time it changes in cpython, use the public `PyGen_GetCode` function. The current implementation would break for Python 3.14 as it has been changed upstream in python/cpython#120835.
@efimov-mikhailefimov-mikhail mentioned this pull request Jun 13, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannonmarkshannonmarkshannon approved these changes

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner

@mdboommdboomAwaiting requested review from mdboom

Assignees

No one assigned

Labels

interpreter-core(Objects, Python, Grammar, and Parser dirs)skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

generator frame type should not be PyObject*[]

4 participants

@iritkatriel@markshannon@bedevere-bot@mdboom