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, <benja...@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/ >
_______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
