-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
[PEP 573] bpo-38787: Module State Access from C Extension Methods #17145
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
Conversation
Objects/typeobject.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Py_XINCREF()?
Objects/typeobject.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the guarantee that prevents the two tuple item accesses (here and below) from running out of bounds?
Could that be worth a comment, at least?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A for loop might be better: first use PyTuple_Size to also check the type, then use the faster PyTuple_GET_ITEM.
encukou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of comments on first glance:
Include/object.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be unnecessary.
Include/methodobject.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the existing PyCFunction_Type for methods?
The METH_METHOD flag should signal that the underlying struct is PyCMethodObject; you'll also want to set tp_itemsize and the use PyObject_GC_NewVar.
Include/methodobject.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks unnecessary; the PEP only mentions PyCFunction_GET_CLASS.
Include/object.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, this is missing from the PEP's summary.
Include/object.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicates the definition in Include/internal/object.h.
Objects/descrobject.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be much simpler, since PyCFunction_NewEx(...) is defined as PyCMethod_New(..., NULL).
Objects/methodobject.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it would allow (METH_VARARGS | METH_METHOD) or METH_FASTCALL | METH_METHOD
Objects/typeobject.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A for loop might be better: first use PyTuple_Size to also check the type, then use the faster PyTuple_GET_ITEM.
Modules/_testmultiphase.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also test with another slot, not only __init__?
Tools/clinic/clinic.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it always named cls?
This is a WIP pull request for PEP 573.
The purpose of this PEP is to provide a way to access per-module state from methods belonging to classes defined in extension modules.
https://bugs.python.org/issue38787