Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2293)

#28544: Implement asyncio.Task in C

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 months ago by yselivanov
Modified:
11 months, 4 weeks ago
Reviewers:
andrew.svetlov, songofacandy
CC:
gvanrossum, brett.cannon, haypo, ned.deily, asvetlov, inada.naoki, Elvis.Pranskevichus, devnull_psf.upfronthosting.co.za, Yury Selivanov
Visibility:
Public.

Patch Set 1 #

Total comments: 10

Patch Set 2 #

Patch Set 3 #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/asyncio/base_events.py View 1 2 1 chunk +1 line, -1 line 0 comments Download
Lib/asyncio/base_futures.py View 1 2 1 chunk +70 lines, -0 lines 0 comments Download
Lib/asyncio/base_tasks.py View 1 2 1 chunk +76 lines, -0 lines 0 comments Download
Lib/asyncio/coroutines.py View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
Lib/asyncio/futures.py View 1 2 3 chunks +15 lines, -68 lines 0 comments Download
Lib/asyncio/tasks.py View 1 2 5 chunks +16 lines, -64 lines 0 comments Download
Lib/test/test_asyncio/test_tasks.py View 1 2 77 chunks +203 lines, -137 lines 0 comments Download
Modules/_asynciomodule.c View 1 2 23 chunks +1612 lines, -279 lines 9 comments Download
Modules/clinic/_asynciomodule.c.h View 1 2 1 chunk +411 lines, -0 lines 0 comments Download

Messages

Total messages: 6
asvetlov
Looks good but please take a look on my comments http://bugs.python.org/review/28544/diff/18976/Lib/test/test_asyncio/test_tasks.py File Lib/test/test_asyncio/test_tasks.py (right): http://bugs.python.org/review/28544/diff/18976/Lib/test/test_asyncio/test_tasks.py#newcode83 ...
11 months, 4 weeks ago #1
Yury Selivanov
Thanks for the review, Andrew! http://bugs.python.org/review/28544/diff/18976/Lib/test/test_asyncio/test_tasks.py File Lib/test/test_asyncio/test_tasks.py (right): http://bugs.python.org/review/28544/diff/18976/Lib/test/test_asyncio/test_tasks.py#newcode83 Lib/test/test_asyncio/test_tasks.py:83: return self.__class__.Task(coro, loop=loop) On ...
11 months, 4 weeks ago #2
asvetlov
http://bugs.python.org/review/28544/diff/18976/Lib/test/test_asyncio/test_tasks.py File Lib/test/test_asyncio/test_tasks.py (right): http://bugs.python.org/review/28544/diff/18976/Lib/test/test_asyncio/test_tasks.py#newcode83 Lib/test/test_asyncio/test_tasks.py:83: return self.__class__.Task(coro, loop=loop) On 2016/10/27 21:22:17, Yury Selivanov wrote: ...
11 months, 4 weeks ago #3
Yury Selivanov
http://bugs.python.org/review/28544/diff/18976/Modules/_asynciomodule.c File Modules/_asynciomodule.c (right): http://bugs.python.org/review/28544/diff/18976/Modules/_asynciomodule.c#newcode1452 Modules/_asynciomodule.c:1452: Py_DECREF(task); On 2016/10/27 21:38:06, asvetlov wrote: > On 2016/10/27 ...
11 months, 4 weeks ago #4
inada.naoki
LGTM. http://bugs.python.org/review/28544/diff/18982/Modules/_asynciomodule.c File Modules/_asynciomodule.c (right): http://bugs.python.org/review/28544/diff/18982/Modules/_asynciomodule.c#newcode912 Modules/_asynciomodule.c:912: (void)FutureObj_clear(fut); Is this for some static checker? http://bugs.python.org/review/28544/diff/18982/Modules/_asynciomodule.c#newcode1184 ...
11 months, 4 weeks ago #5
Yury Selivanov
11 months, 4 weeks ago #6
Thanks for the review, Inada-san. I'll commit the patch soon.

http://bugs.python.org/review/28544/diff/18982/Modules/_asynciomodule.c
File Modules/_asynciomodule.c (right):

http://bugs.python.org/review/28544/diff/18982/Modules/_asynciomodule.c#newco...
Modules/_asynciomodule.c:912: (void)FutureObj_clear(fut);
On 2016/10/28 15:01:08, methane wrote:
> Is this for some static checker?

It's used to turn off "Warn if return value not used/checked" compiler warning.
I think we are still using this trick in CPython code base.

http://bugs.python.org/review/28544/diff/18982/Modules/_asynciomodule.c#newco...
Modules/_asynciomodule.c:1184: if (!PyArg_ParseTuple(args, "O|", &fut)) {
On 2016/10/28 15:01:08, methane wrote:
> "O:_wakeup" ?

Good catch.  I think I'll update this function (and _step) to use AC.

http://bugs.python.org/review/28544/diff/18982/Modules/_asynciomodule.c#newco...
Modules/_asynciomodule.c:1879: } else {
On 2016/10/28 15:01:08, methane wrote:
> PEP7 recommends newline between `}` and `else {`.
Will fix this

http://bugs.python.org/review/28544/diff/18982/Modules/_asynciomodule.c#newco...
Modules/_asynciomodule.c:2090: Py_RETURN_NONE;
On 2016/10/28 15:01:08, methane wrote:
> `return result` may be ok.

Good idea
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7