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

bpo-40421: Cleanup PyFrame C API #32417

Merged
merged 1 commit into from Apr 19, 2022
Merged

bpo-40421: Cleanup PyFrame C API #32417

merged 1 commit into from Apr 19, 2022

Conversation

Copy link
Member

@vstinner vstinner commented Apr 8, 2022

@vstinner
Copy link
Member Author

@vstinner vstinner commented Apr 8, 2022

@markshannon: I started to review #32413 before seeing that it was already merged 3 hours ago, aaaah :-) So I created a PR instead.

I don't think that it's useful to mention that the function argument must not be NULL. We don't do that in the doc of other C API functions. It's implicit.

@vstinner
Copy link
Member Author

@vstinner vstinner commented Apr 8, 2022

In terms of API, PyFrame_GetLasti() returns an int. Would it make sense to return Py_ssize_t?

The code constructor makes sure that co_code cannot be larger than INT_MAX:

    /* Make sure that code is indexable with an int, this is
       a long running assumption in ceval.c and many parts of
       the interpreter. */
    if (PyBytes_GET_SIZE(con->code) > INT_MAX) {
        PyErr_SetString(PyExc_OverflowError,
                        "code: co_code larger than INT_MAX");
        return -1;
    }

@iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Apr 11, 2022

https://bugs.python.org/issue40421 is closed. What is the status of this PR?

@markshannon markshannon reopened this Apr 18, 2022
@markshannon
Copy link
Member

@markshannon markshannon commented Apr 18, 2022

Thanks @vstinner
Looks good to me.

@iritkatriel This is just a minor tidy up, https://bugs.python.org/issue40421 can remain closed.

@markshannon
Copy link
Member

@markshannon markshannon commented Apr 18, 2022

Would it make sense to return Py_ssize_t?

I don't think so.

@vstinner vstinner merged commit aa5c0a9 into python:main Apr 19, 2022
21 of 22 checks passed
@vstinner vstinner deleted the frame branch Apr 19, 2022
@vstinner
Copy link
Member Author

@vstinner vstinner commented Apr 19, 2022

Merged, thanks for the review @markshannon

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

5 participants