<div dir="ltr">Wow, this is an excellent review.  Thank you.<div><br></div><div>My only question is with respect to this:</div><div><br></div><div><span style="font-size:12.8000001907349px">I think there ought to be two opcodes; one for unpacking maps in</span><br style="font-size:12.8000001907349px"><span style="font-size:12.8000001907349px">function calls and another for literals. The whole function location</span><br style="font-size:12.8000001907349px"><span style="font-size:12.8000001907349px">thing is rather hideous.</span><br></div><div><span style="font-size:12.8000001907349px"><br></span></div><div><span style="font-size:12.8000001907349px">What are the two opcodes?  BUILD_MAP_UNPACK and BUILD_MAP_UNPACK_WITH_CALL?</span></div><div><br>The first takes (n) a number of maps that it will merge, and the second does the same but also accepts (function_call_location) for the purpose of error reporting?</div><div><br></div><div>Thanks,</div><div><br></div><div>Neil </div><div><span style="font-size:12.8000001907349px"><br></span></div><div><span style="font-size:12.8000001907349px"><br></span></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 19, 2015 at 1:53 PM,  <span dir="ltr"><<a href="mailto:benjamin@python.org" target="_blank">benjamin@python.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">It's a start.<br>
<br>
There need to be documentation updates.<br>
<br>
There are still unrelated style changes in compile.c that should be<br>
reverted.<br>
<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Include/opcode.h" target="_blank">http://bugs.python.org/review/2292/diff/14152/Include/opcode.h</a><br>
File Include/opcode.h (right):<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Include/opcode.h#newcode71
Include/opcode.h:71" target="_blank">http://bugs.python.org/review/2292/diff/14152/Include/opcode.h#newcode71<br>
Include/opcode.h:71</a>: #define DELETE_GLOBAL              98<br>
This file should not be manually changed. Rather,<br>
Tools/scripts/generate_opcode_h.py should be run.<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Lib/importlib/_bootstrap.py" target="_blank">http://bugs.python.org/review/2292/diff/14152/Lib/importlib/_bootstrap.py</a><br>
File Lib/importlib/_bootstrap.py (right):<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Lib/importlib/_bootstrap.py#newcode428
Lib/importlib/_bootstrap.py:428" target="_blank">http://bugs.python.org/review/2292/diff/14152/Lib/importlib/_bootstrap.py#newcode428<br>
Lib/importlib/_bootstrap.py:428</a>: MAGIC_NUMBER = (3321).to_bytes(2,<br>
'little') + b'\r\n'<br>
As the comment above indicates, the magic value should be incremented by<br>
10 not 1. Also, the comment needs to be updated.<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Lib/test/test_ast.py" target="_blank">http://bugs.python.org/review/2292/diff/14152/Lib/test/test_ast.py</a><br>
File Lib/test/test_ast.py (right):<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Lib/test/test_ast.py#newcode937
Lib/test/test_ast.py:937" target="_blank">http://bugs.python.org/review/2292/diff/14152/Lib/test/test_ast.py#newcode937<br>
Lib/test/test_ast.py:937</a>: main()<br>
Why is this here?<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Lib/test/test_parser.py" target="_blank">http://bugs.python.org/review/2292/diff/14152/Lib/test/test_parser.py</a><br>
File Lib/test/test_parser.py (right):<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Lib/test/test_parser.py#newcode334
Lib/test/test_parser.py:334" target="_blank">http://bugs.python.org/review/2292/diff/14152/Lib/test/test_parser.py#newcode334<br>
Lib/test/test_parser.py:334</a>: self.check_expr('{**{}, 3:4, **{5:6,<br>
7:8}}')<br>
Should there be tests for the new function call syntax, too?<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Python/ast.c" target="_blank">http://bugs.python.org/review/2292/diff/14152/Python/ast.c</a><br>
File Python/ast.c (right):<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Python/ast.c#newcode1746
Python/ast.c:1746" target="_blank">http://bugs.python.org/review/2292/diff/14152/Python/ast.c#newcode1746<br>
Python/ast.c:1746</a>: ast_error(c, n, "iterable unpacking cannot be used in<br>
comprehension");<br>
|n| should probably be |ch|<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Python/ast.c#newcode2022
Python/ast.c:2022" target="_blank">http://bugs.python.org/review/2292/diff/14152/Python/ast.c#newcode2022<br>
Python/ast.c:2022</a>: if (TYPE(CHILD(ch, 0)) == DOUBLESTAR)<br>
int is_dict = TYPE(CHILD(ch, 0)) == DOUBLESTAR;<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Python/ast.c#newcode2026
Python/ast.c:2026" target="_blank">http://bugs.python.org/review/2292/diff/14152/Python/ast.c#newcode2026<br>
Python/ast.c:2026</a>: && TYPE(CHILD(ch, 1)) == COMMA)) {<br>
boolean operators should be on the previous line<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Python/ast.c#newcode2031
Python/ast.c:2031" target="_blank">http://bugs.python.org/review/2292/diff/14152/Python/ast.c#newcode2031<br>
Python/ast.c:2031</a>: && TYPE(CHILD(ch, 1)) == comp_for) {<br>
ditto<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Python/ast.c#newcode2036
Python/ast.c:2036" target="_blank">http://bugs.python.org/review/2292/diff/14152/Python/ast.c#newcode2036<br>
Python/ast.c:2036</a>: && TYPE(CHILD(ch, 3 - is_dict)) == comp_for) {<br>
ditto<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Python/ceval.c" target="_blank">http://bugs.python.org/review/2292/diff/14152/Python/ceval.c</a><br>
File Python/ceval.c (right):<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Python/ceval.c#newcode2403
Python/ceval.c:2403" target="_blank">http://bugs.python.org/review/2292/diff/14152/Python/ceval.c#newcode2403<br>
Python/ceval.c:2403</a>: as_tuple = PyObject_CallFunctionObjArgs(<br>
Use PyList_AsTuple.<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Python/ceval.c#newcode2498
Python/ceval.c:2498" target="_blank">http://bugs.python.org/review/2292/diff/14152/Python/ceval.c#newcode2498<br>
Python/ceval.c:2498</a>: TARGET(BUILD_MAP_UNPACK) {<br>
I think there ought to be two opcodes; one for unpacking maps in<br>
function calls and another for literals. The whole function location<br>
thing is rather hideous.<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Python/ceval.c#newcode2526
Python/ceval.c:2526" target="_blank">http://bugs.python.org/review/2292/diff/14152/Python/ceval.c#newcode2526<br>
Python/ceval.c:2526</a>: if (PySet_Size(intersection)) {<br>
Probably this would be faster with PySet_GET_SIZE(so).<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Python/compile.c" target="_blank">http://bugs.python.org/review/2292/diff/14152/Python/compile.c</a><br>
File Python/compile.c (right):<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Python/compile.c#newcode2931
Python/compile.c:2931" target="_blank">http://bugs.python.org/review/2292/diff/14152/Python/compile.c#newcode2931<br>
Python/compile.c:2931</a>: asdl_seq_SET(elts, i, elt->v.Starred.value);<br>
The compiler should not be mutating the AST.<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Python/compile.c#newcode3088
Python/compile.c:3088" target="_blank">http://bugs.python.org/review/2292/diff/14152/Python/compile.c#newcode3088<br>
Python/compile.c:3088</a>: int i, nseen, nkw = 0;<br>
Many of these should probably be Py_ssize_t.<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Python/symtable.c" target="_blank">http://bugs.python.org/review/2292/diff/14152/Python/symtable.c</a><br>
File Python/symtable.c (right):<br>
<br>
<a href="http://bugs.python.org/review/2292/diff/14152/Python/symtable.c#newcode1372
Python/symtable.c:1372" target="_blank">http://bugs.python.org/review/2292/diff/14152/Python/symtable.c#newcode1372<br>
Python/symtable.c:1372</a>: if (e != NULL) {<br>
Please fix the callers, so they don't pass NULL here.<br>
<br>
<a href="http://bugs.python.org/review/2292/" target="_blank">http://bugs.python.org/review/2292/</a><br>
</blockquote></div><br></div>