Jeffrey Yasskin <jyass...@gmail.com> added the comment: Looking through the patch...
I don't really like the names for JUMP_OR_POP and POP_OR_JUMP. (They don't really indicate to me that the choice depends on the truth of the top of the stack.) How about JUMP_IF_FALSE_OR_POP and JUMP_IF_TRUE_OR_POP (or s/OR/ELSE/)? If the old opcodes weren't called JUMP_IF_XXX, I'd suggest the always-popping variants just use those names. Many other opcodes implicitly consume their arguments so these could too. But since this would be confusing with both the old meanings and the other new pair, your names are probably better. The comments in opcode.h and opcode.py are inconsistent. I now get a warning that PRED_POP_TOP is defined but not used, so you should remove the PREDICTED macro for it. I wonder if BINARY_AND and BINARY_OR should get predictions ... not for this patch. I would probably put the POP_JUMP_IF_XXX definitions next to the JUMP_OR_POP definitions to emphasize their similarity. You missed a comment referring to JUMP_IF_FALSE before compiler_try_except(). POP_JUMP_IF_TRUE is only used in one place: assertions. I wonder if anyone would cry if we compiled assertions to UNARY_NOT; POP_JUMP_IF_FALSE instead... Anyway, also not for this patch. In compiler_comprehension_generator, "compiler_use_next_block(c, skip);" is now always followed by "compiler_use_next_block(c, if_cleanup);". Should you remove the use(skip) call? In peephole.c, s/JUMP_SIGN/JUMPS_ON_TRUE/ ? test_peepholer fails for me, which makes sense because it still refers to JUMP_IF_XXX. Whoo, the peephole.c changes are complex. I'll do those in a second round. _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue4715> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
