gh-120834: Use PyGenObject for generators, coroutines and async generators #120976
Conversation
iritkatriel
commented
Jun 24, 2024
•
iritkatriel
commented
- Issue: generator frame type should not be PyObject*[] #120834
| @@ -35,7 +35,7 @@ PyAPI_FUNC(PyObject *) PyCoro_New(PyFrameObject *, | |||
|
|
|||
| /* --- Asynchronous Generators -------------------------------------------- */ | |||
|
|
|||
| typedef struct _PyAsyncGenObject PyAsyncGenObject; | |||
| typedef PyGenObject PyAsyncGenObject; | |||
There was a problem hiding this comment.
PyCoroObject and PyAsyncGenObject are mentioned in the docs, so I think we need to leave this even though we're not using these names.
| struct _PyCoroObject { | ||
| _PyGenObject_HEAD(cr) | ||
| /* The gi_ prefix is for generator-iterator. */ | ||
| PyObject_HEAD \ |
There was a problem hiding this comment.
We don't need the line extension \s any more.
| @@ -1399,12 +1399,12 @@ PyErr_WarnExplicitFormat(PyObject *category, | |||
| } | |||
|
|
|||
| void | |||
| _PyErr_WarnUnawaitedAgenMethod(PyAsyncGenObject *agen, PyObject *method) | |||
| _PyErr_WarnUnawaitedAgenMethod(PyGenObject *agen, PyObject *method) | |||
There was a problem hiding this comment.
This seems a less correct type. agen should be an async generator, so PyAsyncGenObject * seems the correct type.
There was a problem hiding this comment.
Are we going to keep PyAsyncGenObject and PyCoroObject, now that they are just aliases?
It's not like using them in a signature will give compile-time type checks.
I was assuming we will want to stop using them altogether and deprecate them from the API.
| @@ -1413,7 +1413,7 @@ _PyErr_WarnUnawaitedAgenMethod(PyAsyncGenObject *agen, PyObject *method) | |||
|
|
|||
|
|
|||
| void | |||
| _PyErr_WarnUnawaitedCoroutine(PyObject *coro) | |||
| _PyErr_WarnUnawaitedCoroutine(PyGenObject *coro) | |||
There was a problem hiding this comment.
This should only be called with a coroutine, so I think PyCoroObject * is correct type.
Maybe add an assert that Py_TYPE(coro) == &PyCoro_Type?
| @@ -232,7 +232,7 @@ static inline void _Py_LeaveRecursiveCall(void) { | |||
|
|
|||
| extern struct _PyInterpreterFrame* _PyEval_GetFrame(void); | |||
|
|
|||
| PyAPI_FUNC(PyObject *)_Py_MakeCoro(PyFunctionObject *func); | |||
| PyAPI_FUNC(PyGenObject *)_Py_MakeCoro(PyFunctionObject *func); | |||
There was a problem hiding this comment.
We generally have a convention that there is a correspondence between the C type and the Python class.
e.g. if PyXXX_Check[Exact](obj) is true, then it is safe to cast (PyXXXObject *)obj and vice-versa.
This sort of breaks that for coroutines and async generators.
So, I think it best to leave this as PyObject *. Does that make sense to you?
|
When you're done making the requested changes, leave the comment: |
|
We decided not to do this after all. |
