[Python-Dev] Missing *-unpacking generalizations (issue 2292)

[Neil Girdhar]

Wow, this is an excellent review.  Thank you.

My only question is with respect to this:

I think there ought to be two opcodes; one for unpacking maps in
function calls and another for literals. The whole function location
thing is rather hideous.

What are the two opcodes?  BUILD_MAP_UNPACK and BUILD_MAP_UNPACK_WITH_CALL?

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?

Thanks,

Neil



On Thu, Mar 19, 2015 at 1:53 PM, <benjamin at python.org> wrote:

> It's a start.
>
> There need to be documentation updates.
>
> There are still unrelated style changes in compile.c that should be
> reverted.
>
>
> http://bugs.python.org/review/2292/diff/14152/Include/opcode.h
> File Include/opcode.h (right):
>
> http://bugs.python.org/review/2292/diff/14152/Include/opcode.h#newcode71
> Include/opcode.h:71: #define DELETE_GLOBAL              98
> This file should not be manually changed. Rather,
> Tools/scripts/generate_opcode_h.py should be run.
>
> http://bugs.python.org/review/2292/diff/14152/Lib/importlib/_bootstrap.py
> File Lib/importlib/_bootstrap.py (right):
>
>
> http://bugs.python.org/review/2292/diff/14152/Lib/importlib/_bootstrap.py#newcode428
> Lib/importlib/_bootstrap.py:428: MAGIC_NUMBER = (3321).to_bytes(2,
> 'little') + b'\r\n'
> As the comment above indicates, the magic value should be incremented by
> 10 not 1. Also, the comment needs to be updated.
>
> http://bugs.python.org/review/2292/diff/14152/Lib/test/test_ast.py
> File Lib/test/test_ast.py (right):
>
>
> http://bugs.python.org/review/2292/diff/14152/Lib/test/test_ast.py#newcode937
> Lib/test/test_ast.py:937: main()
> Why is this here?
>
> http://bugs.python.org/review/2292/diff/14152/Lib/test/test_parser.py
> File Lib/test/test_parser.py (right):
>
>
> http://bugs.python.org/review/2292/diff/14152/Lib/test/test_parser.py#newcode334
> Lib/test/test_parser.py:334: self.check_expr('{**{}, 3:4, **{5:6,
> 7:8}}')
> Should there be tests for the new function call syntax, too?
>
> http://bugs.python.org/review/2292/diff/14152/Python/ast.c
> File Python/ast.c (right):
>
> http://bugs.python.org/review/2292/diff/14152/Python/ast.c#newcode1746
> Python/ast.c:1746: ast_error(c, n, "iterable unpacking cannot be used in
> comprehension");
> |n| should probably be |ch|
>
> http://bugs.python.org/review/2292/diff/14152/Python/ast.c#newcode2022
> Python/ast.c:2022: if (TYPE(CHILD(ch, 0)) == DOUBLESTAR)
> int is_dict = TYPE(CHILD(ch, 0)) == DOUBLESTAR;
>
> http://bugs.python.org/review/2292/diff/14152/Python/ast.c#newcode2026
> Python/ast.c:2026: && TYPE(CHILD(ch, 1)) == COMMA)) {
> boolean operators should be on the previous line
>
> http://bugs.python.org/review/2292/diff/14152/Python/ast.c#newcode2031
> Python/ast.c:2031: && TYPE(CHILD(ch, 1)) == comp_for) {
> ditto
>
> http://bugs.python.org/review/2292/diff/14152/Python/ast.c#newcode2036
> Python/ast.c:2036: && TYPE(CHILD(ch, 3 - is_dict)) == comp_for) {
> ditto
>
> http://bugs.python.org/review/2292/diff/14152/Python/ceval.c
> File Python/ceval.c (right):
>
> http://bugs.python.org/review/2292/diff/14152/Python/ceval.c#newcode2403
> Python/ceval.c:2403: as_tuple = PyObject_CallFunctionObjArgs(
> Use PyList_AsTuple.
>
> http://bugs.python.org/review/2292/diff/14152/Python/ceval.c#newcode2498
> Python/ceval.c:2498: TARGET(BUILD_MAP_UNPACK) {
> I think there ought to be two opcodes; one for unpacking maps in
> function calls and another for literals. The whole function location
> thing is rather hideous.
>
> http://bugs.python.org/review/2292/diff/14152/Python/ceval.c#newcode2526
> Python/ceval.c:2526: if (PySet_Size(intersection)) {
> Probably this would be faster with PySet_GET_SIZE(so).
>
> http://bugs.python.org/review/2292/diff/14152/Python/compile.c
> File Python/compile.c (right):
>
> http://bugs.python.org/review/2292/diff/14152/Python/compile.c#newcode2931
> Python/compile.c:2931: asdl_seq_SET(elts, i, elt->v.Starred.value);
> The compiler should not be mutating the AST.
>
> http://bugs.python.org/review/2292/diff/14152/Python/compile.c#newcode3088
> Python/compile.c:3088: int i, nseen, nkw = 0;
> Many of these should probably be Py_ssize_t.
>
> http://bugs.python.org/review/2292/diff/14152/Python/symtable.c
> File Python/symtable.c (right):
>
> http://bugs.python.org/review/2292/diff/14152/Python/symtable.c#newcode1372
> Python/symtable.c:1372: if (e != NULL) {
> Please fix the callers, so they don't pass NULL here.
>
> http://bugs.python.org/review/2292/
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/python-dev/attachments/20150320/38c3a1bf/attachment.html>