On 9/10/19 2:15 PM, Stefan Behnel wrote:
Hi all, and thank you to the authors of this well thought-out PEP.
https://www.python.org/dev/peps/pep-0573/
I read through the latest version and found it in a very good and convincing shape. It has been resting for more than a year now, and some things happened along the way, most notably PEP 590, to which I think it should be adapted. Here are my comments.
I find it a bit sad that state lookups in slot methods are disadvantaged compared to normal methods, given that their entire point is to provide a performance _benefit_. However, for the time being, we do not know how relevant and visible this will become in practice, so it is good that the PEP does not propose a new/real/complex solution to it for now. Instead, it resorts to the MRO lookup (which can always be done), and leaves any larger change open for the future, when we know better if there really is a problem to solve. Check from my side, this is the right trade-off.
I like the idea of immutable exception types.
I do to, but I realized it is orthogonal to the rest of the PEP, and might conflict with other efforts like Eric's conversion to heap types. I would rather leave this to a follow-up PEP. I proposed removal of immutable exceptions in the PR [3].
[...]
Section [1] is the one on extension type methods, which IMHO should be adapted to PEP 590. I think it should be based on the vectorcall/fastcall calling conventions and not take tuple and dict but C arrays as arguments. Alternatively, we could have two new signatures (old-style + new-style), but that feels like overkill. the fastcall signatures are easy enough to handle in C code, and in most cases easier than tuple+dict. The signature of PyCMethod should become
(PyObject *obj, PyTypeObject *defining_class, PyObject *const *args, size_t nargs, PyObject *keywords)
One nitpick I realized now: vectorcall uses a "number of arguments + offset flag" argument, so we now tend to use "nargsf" instead of just "nargs" for the argument name. I added that to the PR.
The descriptor special casing [2] in PEP 590 is also worth mentioning in the context of this PEP. As far as I can see it, the C method type should implement the vectorcall protocol, and then pass the call through to the underlying C function, adding the "defining_class" parameter along the way. I've written up a PR [3] to change the PEP in that direction.
The "_PyMethodDef_RawFastCall*" functions may no longer need to be modified after this, I think.
BTW, if there is "a new type PyCMethodObject", then there is probably also a PyTypeObject for it, right? That should be mentioned somewhere.
That's just a C type (struct) for more storage, it shouldn't be visible from Python.
[...]
Under "Helpers", it says "PyType_GetModuleState … returns pointer to state of the module". I assume that this refers to the PEP-3121 module state, but an explicit reference would make it clearer.
The previous paragraph says "per-module state", to the terminology section. In the PR, I linked to the terminology section.
I personally hate calling things "PEP-XXX Y", because I have trouble remembering numbers. So prefer to not call this "PEP-3121 module state".
It also says in the same section that "When a type without a module is passed in, SystemError is set and NULL returned". Is that necessarily an exception case? Why not simply return NULL and let the user deal with it? I think the question is: is there a use case for getting the module state of an arbitrary type? If so, passing a type that does not have a module reference is fine and should not raise an exception. If that's not a use case, then it _might_ be worth informing the user about their misuse, or not. I don't know. What was the intention here?
I don't know, so I default to not let errors pass silently.
Finally, the reference link to the patch set of the implementation does not work for me. Maybe it should be this?
https://github.com/Dormouse759/cpython/compare/master...Dormouse759:pep-c
It's now https://github.com/Dormouse759/cpython/tree/pep-c-rebase_newer And it's a work in progress.
Also changed in the PR.
As you can see, I do not have any objections or major concerns about this PEP. Next, I'll look through the implementation, and would also like to see at least an attempt to implement it in Cython, to get an idea about how its usage would look in the general case. But apart from that, I can see that a lot of thinking has already gone into this PEP.
Stefan
Thank you for the review!
[1] https://www.python.org/dev/peps/pep-0573/#passing-the-defining-class-to-exte...
[2] https://www.python.org/dev/peps/pep-0590/#descriptor-behavior
[3] https://github.com/python/peps/pull/1159
[4] https://www.python.org/dev/peps/pep-0573/#static-exceptions
capi-sig mailing list -- capi-sig@python.org To unsubscribe send an email to capi-sig-leave@python.org