Nick Coghlan <ncogh...@gmail.com> added the comment:

Finally got around to reviewing this (just a visual scan at this stage) - 
thanks for the effort. These are mostly "big picture" type comments, so I'm 
keeping them here rather than burying them amongst all the details in the code 
review tool.

The effect that collapsing Num/Str/Bytes into a single Lit node type has on 
ast.literal_eval bothered me initially, but looking more closely, I think those 
changes will actually improve the function (string concatenation will now work, 
and errors like "'hello' - 'world'" should give a more informative TypeError). 
(Bikeshed: We use Attribute rather than Attr for that node type, perhaps the 
full "Literal" name would be better, too)

Lib/test/disutil.py should really be made a feature of the dis module itself, 
by creating an inner disassembly function that returns a string, then making 
the existing "dis" and "disassembly" functions print that string (i.e. similar 
to what I already did in making dis.show_code() a thin wrapper around the new 
dis.code_info() function in 3.2). In the absence of a better name, "dis_to_str" 
would do.

Since the disassembly is interpreter specific, the new disassembly tests really 
shouldn't go directly in test_compile.py. A separate "test_ast_optimiser" file 
would be easier for alternate implementations to skip over. A less fragile 
testing strategy may also be to use the ast.PyCF_ONLY_AST flag and check the 
generated AST rather than the generated bytecode.

I'd like to see a written explanation for the first few changes in 
test_peepholer.py. Are those cases no longer optimised? Are they optimised 
differently? Why did these test cases have to change? (The later changes in 
that file look OK, since they seem to just be updating tests to handle the more 
comprehensive optimisation)

When you get around to rebasing the patch on 3.3 trunk, don't forget to drop 
any unneeded "from __future__" imports.

The generated code for the Lit node type looks wrong: it sets v to Py_None, 
then immediately checks to see if v is NULL again.

Don't use "string" as a C type - use "char *" (and "char **" instead of "string 
*").

There should be a new compiler flag to skip the AST optimisation step.

A bunch of the compiler internal changes seem to make the basic flow of the 
generated assembly not match the incoming source code. It doesn't seem like a 
good idea to conflate these with the AST optimisation patch. If that means 
leaving the peepholer in to handle them for now, that's OK - it's fine to just 
descope the peepholer without removing it entirely.

----------

_______________________________________
Python tracker <rep...@bugs.python.org>
<http://bugs.python.org/issue11549>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

Reply via email to