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

vstinner
Copy link
Member

@vstinner vstinner commented Apr 8, 2022

@vstinner
Copy link
Member Author

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

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

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

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
@vstinner vstinner deleted the frame branch April 19, 2022 07:53
@vstinner
Copy link
Member Author

Merged, thanks for the review @markshannon

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.

5 participants