bpo-42085: Introduce dedicated entry in PyAsyncMethods for sending values #22780
Conversation
Include/cpython/object.h
Outdated
| @@ -167,10 +167,23 @@ typedef struct { | |||
| objobjargproc mp_ass_subscript; | |||
| } PyMappingMethods; | |||
|
|
|||
| #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x030A0000 | |||
There was a problem hiding this comment.
cpython/object.h is only imported when Py_LIMITED_API is not defined, so this #if does not make sense. Also, moving the declaration of PySendResult here removes it from the limited API. It actually break the limited API because importing abstract.h with the limited API will be error.
Modules/_asynciomodule.c
Outdated
| static PyObject * | ||
| FutureIter_iternext(futureiterobject *it) | ||
| static PySendResult | ||
| FutureIter_am_send(futureiterobject *it, PyObject *arg, PyObject **result) |
There was a problem hiding this comment.
yes, there is a comment in FutureIter_send, I can copy it here as well:
/* Future.__iter__ doesn't care about values that are pushed to the
* generator, it just returns self.result().
*/
|
@vladima Please add the docs for the new C API. And I think this is ready. |
| @@ -1031,7 +1011,8 @@ static PyMethodDef coro_methods[] = { | |||
| static PyAsyncMethods coro_as_async = { | |||
There was a problem hiding this comment.
Wait, with this PR regular generators no longer benefit from the faster PyIter_Send. I think we should implement am_send for regular generators too.
There was a problem hiding this comment.
Actually we need to address https://github.com/python/cpython/pull/22780/files#r508796665 too.
| typedef struct { | ||
| unaryfunc am_await; | ||
| unaryfunc am_aiter; | ||
| unaryfunc am_anext; | ||
| sendfunc am_send; |
There was a problem hiding this comment.
Should not we add corresponding Py_TPFLAGS_HAVE_ flag for binary compatibility?
There was a problem hiding this comment.
typeslots.h should be updated.
There was a problem hiding this comment.
Should not we add corresponding Py_TPFLAGS_HAVE_ flag for binary compatibility?
Do we support extensions compiled for higher versions of Python in lower versions of Python? If I compile something for 3.10 I wouldn't expect it to work in 3.9.
There was a problem hiding this comment.
IMO we don't need a Py_TPFLAGS_HAVE_ flag and I don't remember adding one for the as_async slot when we implemented PEP 492.
There was a problem hiding this comment.
But if you compile something for 3.9 it is expected to work in 3.10 and do not read uninitialized slot.
There was a problem hiding this comment.
Perhaps there is a bug in the PEP 492 implementation.
There was a problem hiding this comment.
Ahh, I see now, thanks for explaining! Yeah, in this case we do need a flag.
There was a problem hiding this comment.
Perhaps there is a bug in the PEP 492 implementation.
Well, I don't think we need to do anything now that as_async has been there since 3.5. But looking back, maybe we should have added a flag.
There was a problem hiding this comment.
We used an existing slot for PEP-492, so the ABI did not change.
I don't think we necessarily need a new flag. See https://bugs.python.org/issue32388
Objects/typeobject.c
Outdated
| /* Consistency check for Py_TPFLAGS_HAVE_SEND - flag requires | ||
| * type->tp_as_async->am_send to be present. | ||
| */ | ||
| if (type->tp_flags & Py_TPFLAGS_HAVE_SEND) { |
There was a problem hiding this comment.
I'd be more specific with the flag name.
| if (type->tp_flags & Py_TPFLAGS_HAVE_SEND) { | |
| if (type->tp_flags & Py_TPFLAGS_HAVE_AM_SEND) { |
Modules/_asynciomodule.c
Outdated
| static PyObject * | ||
| FutureIter_iternext(futureiterobject *it) | ||
| static PySendResult | ||
| FutureIter_am_send(futureiterobject *it, PyObject *Py_UNUSED(arg), PyObject **result) |
There was a problem hiding this comment.
Need to keep lines under 79 characters
|
@serhiy-storchaka could you please review again? |
|
@pablogsal Pablo maybe you can take a quick look at this? |
I can try to give it a look next week if I find some time. |
|
@scoder @serhiy-storchaka Guys, could you please take another look? I plan to merge this soon. |
|
Alright, I'm going to go ahead and merge this. @serhiy-storchaka and @scoder if you want to further discuss this let's continue on bpo and in follow up PRs. Thanks to everybody for reviews! |
|
@1st1: Please replace |
As in title: create
am_sendslot inPyAsyncMethodsto send values.Skipping new section since I expect a lot of changes as the result of discussions
https://bugs.python.org/issue42085