bpo-43693: Add _PyCode_New() and do some related cleanup.#26258
bpo-43693: Add _PyCode_New() and do some related cleanup.#26258ericsnowcurrently wants to merge 44 commits into
Conversation
|
I agree that we should add a non-API function for creating code objects and Passing a struct as a single parameter to a function means that you have to ensure that all future versions can handling trailing zeroes. PyObject *make_thing(struct thing *t);Version 1: Version 2: Any client code written for version 1 will compile for version 2 and then crash mysteriously when run. |
Sorry, something went wrong.
|
I see your point but consider it less of an issue for an internal-only API like this. For the most part this PR is about addressing the pain points I ran into while making changes around PyCodeObject:
I'm okay with dialing back the changes if you think they aren't worth it, but the definitely are based in my experience thus far with PyCodeObject. |
Sorry, something went wrong.
|
I think we all agree that the API for making PyCodeObjects shouldn't have been public in the first place. But it is 😞 The accessor macros, Ultimately we want something like https://github.com/markshannon/peps/blob/pep-mappable-pyc-file/pep-066x.rst#runtime-objects |
Sorry, something went wrong.
23ec578 to
2a3de5d
Compare
May 20, 2021 17:26
|
Please remove construction of a CodeObject using a structure. As I explained earlier it isn't safe. |
Sorry, something went wrong.
bl-ue
left a comment
There was a problem hiding this comment.
Hi, I found a few things worth taking a look at :)
Sorry, something went wrong.
That only applies for public APIs. This is a purely internal API. The compiler will catch the case you worry about in that explanation.
It allows us something not entirely unlike keyword arguments in C, which I find a big gain to readability. |
Sorry, something went wrong.
|
No, the compiler won't catch it. It is legal to leave trailing fields off a struct assignment. |
Sorry, something went wrong.
|
@markshannon, calling Regardless, there isn't a lot of value to a long discussion about this. If you feel strongly about this then I'll change it to a function with many parameters. |
Sorry, something went wrong.
markshannon
left a comment
There was a problem hiding this comment.
This PR is too big for me to be confident that I'm not missing something important.
Why does it need to be so large?
There are a lot of changes to frameobject.c, typeobject.c and ceval.c that seem unrelated.
Several of which look like they will degrade performance.
Sorry, something went wrong.
|
When you're done making the requested changes, leave the comment: |
Sorry, something went wrong.
|
If this PR is too big to review well enough then I'll split it up. |
Sorry, something went wrong.
The key change here is the addition of
_PyCode_New(), which is an internal-only API to replace use ofPyCode_NewWithPosOnlyArgs(). The problem with that older API is the it has so many parameters. This becomes especially painful when modifying its signature. The new API uses a single struct to hold the values needed to create a newPyCodeObjectinstance.Other changes:
co_nfastlocals,co_ncellvars, andco_nfreevars; this matters because I plan on replacingco_varnames, etc. with a single consolidated tupleco_varnames, etc. from ceval.c to codeobject.c (as functions); this reduced coupling makes it easier to change howPyCodeObjectworks internallyPyCodeObjectfields so they are grouped a bit more logicallyThere is still code in ceval.c (
_PyEval_MakeFrameVector()specifically, and pyframeobject.c that is coupled to the internal structure of PyCodeObject, but this PR is big enough already. 🙂https://bugs.python.org/issue43693