Author ncoghlan
Recipients Trundle, alex, benjamin.peterson, brett.cannon, daniel.urban, dmalcolm, eltoder, georg.brandl, mark.dickinson, nadeem.vawda, ncoghlan, pitrou, rhettinger, santoso.wijaya, terry.reedy
Date 2011-03-27.11:15:23
SpamBayes Score 0.0
Marked as misclassified No
Message-id <1301224524.2.0.946437170881.issue11549@psf.upfronthosting.co.za>
In-reply-to
Content
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.
History
Date User Action Args
2011-03-27 11:15:24ncoghlansetrecipients: + ncoghlan, brett.cannon, georg.brandl, rhettinger, terry.reedy, mark.dickinson, pitrou, nadeem.vawda, benjamin.peterson, alex, Trundle, dmalcolm, daniel.urban, santoso.wijaya, eltoder
2011-03-27 11:15:24ncoghlansetmessageid: <1301224524.2.0.946437170881.issue11549@psf.upfronthosting.co.za>
2011-03-27 11:15:23ncoghlanlinkissue11549 messages
2011-03-27 11:15:23ncoghlancreate