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.

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

GH-100719: Remove redundant gi_code field from generator object. #100749

Merged
merged 10 commits into from
Feb 23, 2023

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Jan 4, 2023

@@ -1962,44 +1962,6 @@ initialize_locals(PyThreadState *tstate, PyFunctionObject *func,
return -1;
}

/* Consumes references to func, locals and all the args */
Copy link
Member

Choose a reason for hiding this comment

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

Why was this moved downwards?

Copy link
Member Author

Choose a reason for hiding this comment

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

There was a small change, it now calls clear_thread_frame() rather than _PyEvalFrameClearAndPop(), so now needs to come after clear_thread_frame

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

mostly LGTM

There's one minor comment, but I'll leave the resolution up to you.

Include/cpython/genobject.h Show resolved Hide resolved
@markshannon
Copy link
Member Author

I've converted this to a draft because I want to make the following changes first.

  • Change the _PyInterpreterFrame so that the code object is the first field.
  • Add back the gi_code field as a union overlapping the frame, ensuring that code accessing gi_code will still work, and allowing us to deprecate the field.

@markshannon markshannon marked this pull request as draft January 6, 2023 11:08
@markshannon markshannon marked this pull request as ready for review February 13, 2023 16:06
Copy link
Member

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

I'm removing the gi_code attribute.
Any C code that accesses the gi_code attribute might clear it, resulting in a double free. It is safer to just remove it.

@markshannon markshannon merged commit 22b8d77 into python:main Feb 23, 2023
scoder added a commit to cython/cython that referenced this pull request Mar 31, 2023
scoder added a commit to cython/cython that referenced this pull request Apr 2, 2023
@markshannon markshannon deleted the trim-code-object-2 branch September 26, 2023 12:50
JelleZijlstra pushed a commit to JelleZijlstra/cpython that referenced this pull request Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants