Author jyasskin
Recipients collinwinter, jyasskin, pitrou
Date 2009-01-14.02:48:35
SpamBayes Score 0.00300923
Marked as misclassified No
Message-id <>
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

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 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

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

Whoo, the peephole.c changes are complex. I'll do those in a second round.
Date User Action Args
2009-01-14 02:48:38jyasskinsetrecipients: + jyasskin, collinwinter, pitrou
2009-01-14 02:48:38jyasskinsetmessageid: <>
2009-01-14 02:48:37jyasskinlinkissue4715 messages
2009-01-14 02:48:36jyasskincreate