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-118609: Add proper error check for framelocalsproxy #118615

Merged
merged 2 commits into from
May 6, 2024

Conversation

gaogaotiantian
Copy link
Member

@gaogaotiantian gaogaotiantian commented May 5, 2024

  • Completed error checks in frameobject.c
  • Fixed the compiler warning for unused variables
  • Polished the declarations a bit so the error checks are closer to the declared variable, and variables are declared in the necessary scope

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Thanks! One style nit, you can fix it or leave it.

(Maybe @sobolevn can review too, in case I missed anything -- I realize I am not up to speed with all the details of writing this kind of code any more.)

Comment on lines 383 to 385
Py_ReprLeave(self);

Py_DECREF(dct);
Copy link
Member

Choose a reason for hiding this comment

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

I'd swap these two lines, so that the Py_ReprEnter/Leave and the PyDict_New/Py_DECREF "brackets" properly nest. But this works too of course.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that makes sense. DECREF dct as soon as we are done with it and then leave repr. Fixed it.

@gvanrossum
Copy link
Member

(Maybe @sobolevn can review too, in case I missed anything -- I realize I am not up to speed with all the details of writing this kind of code any more.)

I meant @Eclips4 of course, since he reported the issue. (But Nikita is also welcome! :-)

@gvanrossum
Copy link
Member

I'll merge everything Monday morning (Pacific time) unless anyone says otherwise.

Copy link
Member

@Eclips4 Eclips4 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thank you!

@gvanrossum gvanrossum merged commit 7528b84 into python:main May 6, 2024
34 checks passed
@gaogaotiantian gaogaotiantian deleted the fix-frameobject-error-checks branch May 6, 2024 17:29
SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants