Review issue 4715 from the public Python tracker

[Collin Winter, 2009-02-27 21:33:21]

Other than these little nits, looks good.

http://codereview.appspot.com/20094/diff/1/6
File Include/opcode.h (right):

http://codereview.appspot.com/20094/diff/1/6#newcode109
Line 109: #define JUMP_IF_FALSE_OR_POP 111	/* Target byte offset from beginning
of code */
More than 80 chars.

http://codereview.appspot.com/20094/diff/1/12
File Lib/test/test_peepholer.py (right):

http://codereview.appspot.com/20094/diff/1/12#newcode22
Line 22: # UNARY_NOT POP_JUMP_IF_FALSE  -->  POP_JUMP_IF_TRUE'
Can you kill the lingering ' at the end of the line?

http://codereview.appspot.com/20094/diff/1/12#newcode29
Line 29: for elem in ('POP_JUMP_IF_TRUE',):
No need for this for loop.

http://codereview.appspot.com/20094/diff/1/5
File Python/compile.c (right):

http://codereview.appspot.com/20094/diff/1/5#newcode1625
Line 1625: compiler_use_next_block(c, next);
Mixed tabs and spaces?

http://codereview.appspot.com/20094/diff/1/5#newcode1842
Line 1842: [tb, val, exc]	Ln+1:	END_FINALLY	# re-raise exception
Mixed tabs and spaces?

[Jeffrey Yasskin, 2009-02-28 01:39:00]

Note that the original patch was at http://codereview.appspot.com/20064. I've
updated the code at that one.

http://codereview.appspot.com/20094/diff/1/6
File Include/opcode.h (right):

http://codereview.appspot.com/20094/diff/1/6#newcode109
Line 109: #define JUMP_IF_FALSE_OR_POP 111	/* Target byte offset from beginning
of code */
On 2009/02/27 21:33:21, Collin Winter wrote:
> More than 80 chars.

Fixed

http://codereview.appspot.com/20094/diff/1/12
File Lib/test/test_peepholer.py (right):

http://codereview.appspot.com/20094/diff/1/12#newcode22
Line 22: # UNARY_NOT POP_JUMP_IF_FALSE  -->  POP_JUMP_IF_TRUE'
On 2009/02/27 21:33:21, Collin Winter wrote:
> Can you kill the lingering ' at the end of the line?

Sure.

http://codereview.appspot.com/20094/diff/1/12#newcode29
Line 29: for elem in ('POP_JUMP_IF_TRUE',):
On 2009/02/27 21:33:21, Collin Winter wrote:
> No need for this for loop.

True, but the other similar assertions in other tests have it. Removed anyway.

http://codereview.appspot.com/20094/diff/1/5
File Python/compile.c (right):

http://codereview.appspot.com/20094/diff/1/5#newcode1625
Line 1625: compiler_use_next_block(c, next);
On 2009/02/27 21:33:21, Collin Winter wrote:
> Mixed tabs and spaces?

Hm, they seem to be all tabs in my copy. Maybe I fixed it after uploading this.

http://codereview.appspot.com/20094/diff/1/5#newcode1842
Line 1842: [tb, val, exc]	Ln+1:	END_FINALLY	# re-raise exception
On 2009/02/27 21:33:21, Collin Winter wrote:
> Mixed tabs and spaces?

Again, not in my local copy. I'm not sure how it's deciding to flag this. The
over-indented DUP above and the normally-indented opcodes before it all have
tabs before them, just like this END_FINALLY.

[Collin Winter, 2009-03-01 20:08:56]

LGTM sent on the http://codereview.appspot.com/20064 thread. Closing this issue.
Sorry I didn't see the other review issue.