Skip to content

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

Merged
iritkatriel merged 6 commits into
python:mainfrom
iritkatriel:gi_frame-type
Jun 24, 2024
Merged

gh-120834: fix type of *_iframe field in _PyGenObject_HEAD declaration#120835
iritkatriel merged 6 commits into
python:mainfrom
iritkatriel:gi_frame-type

Conversation

@iritkatriel

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

Copy link
Copy Markdown
Member

@bedevere-appbedevere-appBot mentioned this pull request Jun 21, 2024
Closed
iritkatriel
iritkatriel commented Jun 21, 2024
View reviewed changes
Comment threadInclude/cpython/genobject.h Outdated
@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

@mdboommdboom left a comment

Copy link
Copy Markdown
Contributor

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

@markshannonmarkshannon left a comment

Copy link
Copy Markdown
Member

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.

Comment threadInclude/cpython/genobject.h Outdated
Comment threadObjects/genobject.c Outdated
Comment threadObjects/genobject.c Outdated
Comment threadObjects/genobject.c Outdated
Comment threadObjects/genobject.c Outdated
@bedevere-app

bedevere-appBot commented Jun 21, 2024

Copy link
Copy Markdown

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

@iritkatriel

iritkatriel commented Jun 21, 2024

Copy link
Copy Markdown
MemberAuthor

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

markshannon commented Jun 21, 2024•
edited
Loading

Copy link
Copy Markdown
Member

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
Comment threadInclude/internal/pycore_genobject.h
@iritkatriel

iritkatriel commented Jun 21, 2024

Copy link
Copy Markdown
MemberAuthor

I have made the requested changes; please review again.

@bedevere-app

bedevere-appBot commented Jun 21, 2024

Copy link
Copy Markdown

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

@markshannonmarkshannon left a comment

Copy link
Copy Markdown
Member

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

bedevere-bot commented Jun 23, 2024

Copy link
Copy Markdown

🤖 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

iritkatriel commented Jun 23, 2024

Copy link
Copy Markdown
MemberAuthor

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

markshannon commented Jun 24, 2024

Copy link
Copy Markdown
Member

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.

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

@markshannonmarkshannonJun 24, 2024

Copy link
Copy Markdown
Member

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

@iritkatrieliritkatrielJun 24, 2024

Copy link
Copy Markdown
MemberAuthor

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

iritkatriel commented Jun 24, 2024

Copy link
Copy Markdown
MemberAuthor

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

markshannon commented Jun 24, 2024

Copy link
Copy Markdown
Member

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