Exception state lives too long in 3.0

See http://mail.python.org/pipermail/python-3000/2008-March/012830.html
: the exception state survives after the end of the except block although the
py3k spec mandates otherwise.

[Antoine Pitrou]

http://codereview.appspot.com/842/diff/1/21
File Python/ceval.c (right):

http://codereview.appspot.com/842/diff/1/21#newcode1486
Line 1486: */
Hmm, it looks like I have gone past the 79-column limit in this comment block.

[108414_gmail.com]

test

http://codereview.appspot.com/842/diff/1/22
File Lib/test/test_exceptions.py (right):

http://codereview.appspot.com/842/diff/1/22#newcode404
Line 404: def testExceptionCleanupNames(self):
This is a test.

[Jeffrey Yasskin]

Otherwise, looks good to me.

http://codereview.appspot.com/842/diff/1/22
File Lib/test/test_exceptions.py (right):

http://codereview.appspot.com/842/diff/1/22#newcode436
Line 436: obj = None
Comment that this line exists to kill the reference to MyObj. Alternately, use:
  del obj
  self.assertEqual(wr(), None)
Which also avoids the need for the message parameter.

Just for future reference, str(obj) is probably better than "%s"%obj.

[jhylton]

http://codereview.appspot.com/842/diff/1/22
File Lib/test/test_exceptions.py (right):

http://codereview.appspot.com/842/diff/1/22#newcode413
Line 413: self.failIf('e' in locals())
This test isn't too obvious.  The del e seems like to should guarantee that 'e'
is not in locals().  Why would it be in locals()?

http://codereview.appspot.com/842/diff/1/22#newcode428
Line 428: raise MyException(obj)
Are both references necessary?  Will the test fail if only one of them is
created?

http://codereview.appspot.com/842/diff/1/21
File Python/ceval.c (right):

http://codereview.appspot.com/842/diff/1/21#newcode1485
Line 1485: See #2507.
We don't have a lot of comments that refer to specific bug reports.  I think
it's more common to refer to the bug in the checkin message, but omit it from
the code.

http://codereview.appspot.com/842/diff/1/21#newcode1491
Line 1491: assert(tstate->frame->f_exc_traceback == NULL);
I'd move the asserts outside of the else.  They should always be true,
regardless of whether reset_exc_info() called.

[Antoine Pitrou]

http://codereview.appspot.com/842/diff/1/22
File Lib/test/test_exceptions.py (right):

http://codereview.appspot.com/842/diff/1/22#newcode413
Line 413: self.failIf('e' in locals())
On 2008/05/06 12:59:48, jhylton wrote:
> This test isn't too obvious.  The del e seems like to should guarantee that
'e'
> is not in locals().  Why would it be in locals()?

Honestly I don't know. This test is not part of the patch, I've just made sure
the comment is a bit more precise since "exceptions are cleaned up" is a bit
vague :-)

http://codereview.appspot.com/842/diff/1/22#newcode428
Line 428: raise MyException(obj)
On 2008/05/06 12:59:48, jhylton wrote:
> Are both references necessary?  Will the test fail if only one of them is
> created?

One of the references should be located in the exception traceback (via the
locals dictionary), the other in the exception value. This makes the test more
comprehensive than with only one reference. Obviously it will still pass if I
remove one of the references.

http://codereview.appspot.com/842/diff/1/21
File Python/ceval.c (right):

http://codereview.appspot.com/842/diff/1/21#newcode1485
Line 1485: See #2507.
On 2008/05/06 12:59:48, jhylton wrote:
> We don't have a lot of comments that refer to specific bug reports.  I think
> it's more common to refer to the bug in the checkin message, but omit it from
> the code. 

But is it a problem? Mentioning the bug number makes it much easier for a reader
to figure out why this code snippet is there. You can't rely on "svn blame" to
give you the right answer because the snippet might have been modified later for
whatever reason.

http://codereview.appspot.com/842/diff/1/21#newcode1491
Line 1491: assert(tstate->frame->f_exc_traceback == NULL);
On 2008/05/06 12:59:48, jhylton wrote:
> I'd move the asserts outside of the else.  They should always be true,
> regardless of whether reset_exc_info() called.

Well I've copied the code snippet from the end of the eval function, where
similar cleanup is done. I'm not sure why the original author felt the need to
add the asserts in the first place.