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

#26647: Wordcode

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 8 months ago by gunkmute
Modified:
1 year, 6 months ago
Reviewers:
victor.stinner, shadowranger+python, ghost.adh, storchaka, storchaka+cpython
CC:
brett.cannon, Georg, rhettinger, Nick Coghlan, haypo, Benjamin Peterson, devnull_psf.upfronthosting.co.za, storchaka, Yury Selivanov, abarnert, josh.rosenberg, serprex, rmay31_gmail.com
Visibility:
Public.

Patch Set 1 #

Total comments: 33

Patch Set 2 #

Total comments: 6

Patch Set 3 #

Total comments: 22

Patch Set 4 #

Total comments: 7

Patch Set 5 #

Patch Set 6 #

Patch Set 7 #

Total comments: 27

Patch Set 8 #

Total comments: 24

Patch Set 9 #

Total comments: 18

Patch Set 10 #

Patch Set 11 #

Total comments: 5

Patch Set 12 #

Total comments: 4

Patch Set 13 #

Patch Set 14 #

Total comments: 2

Patch Set 15 #

Patch Set 16 #

Patch Set 17 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/dis.rst View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +3 lines, -3 lines 0 comments Download
Include/code.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +13 lines, -0 lines 0 comments Download
Lib/asyncio/coroutines.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -1 line 0 comments Download
Lib/dis.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +3 lines, -3 lines 0 comments Download
Lib/importlib/_bootstrap_external.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +2 lines, -1 line 0 comments Download
Lib/test/test_dis.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 10 chunks +259 lines, -257 lines 0 comments Download
Objects/frameobject.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 7 chunks +12 lines, -10 lines 0 comments Download
Objects/genobject.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +3 lines, -3 lines 0 comments Download
Python/ceval.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 10 chunks +20 lines, -30 lines 0 comments Download
Python/compile.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +6 lines, -6 lines 0 comments Download
Python/importlib.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 38 chunks +1128 lines, -1129 lines 0 comments Download
Python/importlib_external.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 16 chunks +1916 lines, -1918 lines 0 comments Download
Python/peephole.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 30 chunks +104 lines, -101 lines 0 comments Download
Python/wordcode_helpers.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +19 lines, -16 lines 0 comments Download

Messages

Total messages: 28
haypo
Ok, here is a first set of comments. Please move all your unrelated changes to ...
1 year, 8 months ago #1
serprex
http://bugs.python.org/review/26647/diff/16839/Lib/asyncio/coroutines.py File Lib/asyncio/coroutines.py (right): http://bugs.python.org/review/26647/diff/16839/Lib/asyncio/coroutines.py#newcode177 Lib/asyncio/coroutines.py:177: if frame is not None and frame.f_lasti < 0: ...
1 year, 8 months ago #2
josh.rosenberg
Mostly suggestions for reducing churn to minimize diff, but there is one issue in wordcode_helpers.h ...
1 year, 8 months ago #3
haypo
http://bugs.python.org/review/26647/diff/16863/Lib/dis.py File Lib/dis.py (right): http://bugs.python.org/review/26647/diff/16863/Lib/dis.py#newcode217 Lib/dis.py:217: if self.opcode >= HAVE_ARGUMENT: This change doesn't look good ...
1 year, 8 months ago #4
serprex
http://bugs.python.org/review/26647/diff/16863/Lib/test/test_dis.py File Lib/test/test_dis.py (left): http://bugs.python.org/review/26647/diff/16863/Lib/test/test_dis.py#oldcode270 Lib/test/test_dis.py:270: self.assertEqual(dis.opmap["STORE_NAME"], dis.HAVE_ARGUMENT) On 2016/04/01 18:53:56, haypo wrote: > Why ...
1 year, 8 months ago #5
haypo
http://bugs.python.org/review/26647/diff/16875/Lib/dis.py File Lib/dis.py (right): http://bugs.python.org/review/26647/diff/16875/Lib/dis.py#newcode383 Lib/dis.py:383: extended_arg = 0 I suggest to move this line ...
1 year, 8 months ago #6
haypo
http://bugs.python.org/review/26647/diff/16959/Lib/importlib/_bootstrap_external.py File Lib/importlib/_bootstrap_external.py (right): http://bugs.python.org/review/26647/diff/16959/Lib/importlib/_bootstrap_external.py#newcode234 Lib/importlib/_bootstrap_external.py:234: MAGIC_NUMBER = (3361).to_bytes(2, 'little') + b'\r\n' Hum, you must ...
1 year, 8 months ago #7
SilentGhost
https://bugs.python.org/review/26647/diff/16875/Lib/dis.py File Lib/dis.py (right): https://bugs.python.org/review/26647/diff/16875/Lib/dis.py#newcode378 Lib/dis.py:378: label = (arg+i+2 if op in hasjrel This could ...
1 year, 8 months ago #8
serprex
http://bugs.python.org/review/26647/diff/16959/Python/ceval.c File Python/ceval.c (left): http://bugs.python.org/review/26647/diff/16959/Python/ceval.c#oldcode1038 Python/ceval.c:1038: #define PREDICTED_WITH_ARG(op) PRED_##op: oparg = PEEKARG(); next_instr += 3 ...
1 year, 8 months ago #9
haypo
https://bugs.python.org/review/26647/diff/16959/Python/ceval.c File Python/ceval.c (left): https://bugs.python.org/review/26647/diff/16959/Python/ceval.c#oldcode1038 Python/ceval.c:1038: #define PREDICTED_WITH_ARG(op) PRED_##op: oparg = PEEKARG(); next_instr += 3 ...
1 year, 8 months ago #10
serprex
http://bugs.python.org/review/26647/diff/16959/Python/ceval.c File Python/ceval.c (left): http://bugs.python.org/review/26647/diff/16959/Python/ceval.c#oldcode891 Python/ceval.c:891: #define TARGET_WITH_IMPL(op, impl) \ TARGET_WITH_IMPL can be completely replaced ...
1 year, 8 months ago #11
storchaka_gmail.com
This is incomplete review! I still haven't finished reviewing peephole.c. http://bugs.python.org/review/26647/diff/16959/Doc/library/dis.rst File Doc/library/dis.rst (right): http://bugs.python.org/review/26647/diff/16959/Doc/library/dis.rst#newcode34 ...
1 year, 7 months ago #12
serprex
http://bugs.python.org/review/26647/diff/16959/Python/ceval.c File Python/ceval.c (left): http://bugs.python.org/review/26647/diff/16959/Python/ceval.c#oldcode891 Python/ceval.c:891: #define TARGET_WITH_IMPL(op, impl) \ On 2016/04/26 21:56:20, storchaka wrote: ...
1 year, 7 months ago #13
haypo
Thanks for the review Serhiy. I added some more comments ;-) http://bugs.python.org/review/26647/diff/16959/Doc/library/dis.rst File Doc/library/dis.rst (right): ...
1 year, 7 months ago #14
storchaka_gmail.com
http://bugs.python.org/review/26647/diff/16959/Doc/library/dis.rst File Doc/library/dis.rst (right): http://bugs.python.org/review/26647/diff/16959/Doc/library/dis.rst#newcode34 Doc/library/dis.rst:34: 2 LOAD_FAST 0 (alist) On 2016/04/29 08:54:02, haypo wrote: ...
1 year, 7 months ago #15
serprex
http://bugs.python.org/review/26647/diff/17115/Lib/test/test_dis.py File Lib/test/test_dis.py (right): http://bugs.python.org/review/26647/diff/17115/Lib/test/test_dis.py#newcode63 Lib/test/test_dis.py:63: 3 LOAD_CONST 1 (1) Oops, I forgot to get ...
1 year, 7 months ago #16
haypo
The overall change LGTM, but I'm no more 100% sure that starting f_lasti=-1 is safe. ...
1 year, 7 months ago #17
storchaka_gmail.com
Found errors in peephole.c. Reviewing peephole.c still is not completed. http://bugs.python.org/review/26647/diff/17115/Lib/dis.py File Lib/dis.py (right): http://bugs.python.org/review/26647/diff/17115/Lib/dis.py#newcode377 ...
1 year, 7 months ago #18
serprex
http://bugs.python.org/review/26647/diff/17115/Python/ceval.c File Python/ceval.c (right): http://bugs.python.org/review/26647/diff/17115/Python/ceval.c#newcode2066 Python/ceval.c:2066: f->f_lasti -= 2; On 2016/04/30 16:03:14, haypo wrote: > ...
1 year, 7 months ago #19
serprex
http://bugs.python.org/review/26647/diff/17115/Python/peephole.c File Python/peephole.c (left): http://bugs.python.org/review/26647/diff/17115/Python/peephole.c#oldcode401 Python/peephole.c:401: /* Verify that RETURN_VALUE terminates the codestring. This allows ...
1 year, 7 months ago #20
storchaka
http://bugs.python.org/review/26647/diff/17227/Doc/library/dis.rst File Doc/library/dis.rst (right): http://bugs.python.org/review/26647/diff/17227/Doc/library/dis.rst#newcode1015 Doc/library/dis.rst:1015: opcodes which ignore arguments ``< HAVE_ARGUMENT`` and those which ...
1 year, 6 months ago #21
serprex
http://bugs.python.org/review/26647/diff/17227/Python/peephole.c File Python/peephole.c (right): http://bugs.python.org/review/26647/diff/17227/Python/peephole.c#newcode466 Python/peephole.c:466: opcode = codestr[i]; On 2016/05/20 11:09:36, storchaka wrote: > ...
1 year, 6 months ago #22
serprex
http://bugs.python.org/review/26647/diff/17227/Python/peephole.c File Python/peephole.c (right): http://bugs.python.org/review/26647/diff/17227/Python/peephole.c#newcode664 Python/peephole.c:664: break; On 2016/05/21 02:22:16, serprex wrote: > On 2016/05/20 ...
1 year, 6 months ago #23
storchaka
http://bugs.python.org/review/26647/diff/17227/Python/peephole.c File Python/peephole.c (right): http://bugs.python.org/review/26647/diff/17227/Python/peephole.c#newcode466 Python/peephole.c:466: opcode = codestr[i]; On 2016/05/21 02:22:16, serprex wrote: > ...
1 year, 6 months ago #24
storchaka
http://bugs.python.org/review/26647/diff/17227/Python/peephole.c File Python/peephole.c (right): http://bugs.python.org/review/26647/diff/17227/Python/peephole.c#newcode664 Python/peephole.c:664: break; On 2016/05/21 22:47:26, storchaka wrote: > On 2016/05/21 ...
1 year, 6 months ago #25
storchaka
http://bugs.python.org/review/26647/diff/17306/Python/peephole.c File Python/peephole.c (right): http://bugs.python.org/review/26647/diff/17306/Python/peephole.c#newcode159 Python/peephole.c:159: /* Replace LOAD_CONST c1, LOAD_CONST c2 ... LOAD_CONST cn ...
1 year, 6 months ago #26
storchaka
http://bugs.python.org/review/26647/diff/17317/Python/peephole.c File Python/peephole.c (right): http://bugs.python.org/review/26647/diff/17317/Python/peephole.c#newcode44 Python/peephole.c:44: assert(PyList_GET_SIZE(consts) > get_arg(codestr, i)); \ warning: comparison between signed ...
1 year, 6 months ago #27
storchaka
1 year, 6 months ago #28
http://bugs.python.org/review/26647/diff/17319/Python/peephole.c
File Python/peephole.c (right):

http://bugs.python.org/review/26647/diff/17319/Python/peephole.c#newcode696
Python/peephole.c:696: if (j < 0)           /* No backward relative jumps */
warning: comparison of unsigned expression < 0 is always false

http://bugs.python.org/review/26647/diff/17319/Python/peephole.c#newcode733
Python/peephole.c:733: assert(0 <= offset_delta && offset_delta <= 255);
warning: comparison of unsigned expression >= 0 is always true
Sign in to reply to this message.

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