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.

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

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

Merged
merged 6 commits into from
Jun 24, 2024
Merged

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

merged 6 commits into from
Jun 24, 2024
+81 −79

Conversation

iritkatriel
Copy link
Member

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

@bedevere-app bedevere-app bot mentioned this pull request Jun 21, 2024
Closed
@iritkatriel iritkatriel 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

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

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

Include/cpython/genobject.h Outdated Show resolved Hide resolved
Objects/genobject.c Outdated Show resolved Hide resolved
Objects/genobject.c Outdated Show resolved Hide resolved
Objects/genobject.c Outdated Show resolved Hide resolved
Objects/genobject.c Outdated Show resolved Hide resolved
@bedevere-app
Copy link

bedevere-app bot 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
Member Author

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
Copy link
Member Author

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot 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

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

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

🤖 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-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 23, 2024
@iritkatriel
Copy link
Member Author

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

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:main Jun 24, 2024
70 checks passed
markshannon
markshannon reviewed Jun 24, 2024
View reviewed changes
Objects/genobject.c
@@ -814,8 +813,7 @@ static PyAsyncMethods gen_as_async = {
PyTypeObject PyGen_Type = {
PyVarObject_HEAD_INIT(&PyType_Type, 0)
"generator", /* tp_name */
offsetof(PyGenObject, gi_iframe) +
offsetof(_PyInterpreterFrame, localsplus), /* tp_basicsize */
sizeof(PyGenObject), /* tp_basicsize */
Copy link
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

Copy link
Member Author

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

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?

@iritkatriel iritkatriel mentioned this pull request Jun 24, 2024
Merged
@markshannon
Copy link
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)
@cdce8p cdce8p 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markshannon markshannon markshannon approved these changes

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

@mdboom mdboom Awaiting 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