Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(95)

Issue 842: Exception state lives too long in 3.0 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
18 years, 1 month ago by Antoine Pitrou
Modified:
16 years, 10 months ago
Reviewers:
Base URL:
http://svn.python.org/view/*checkout*/python/branches/py3k/
Visibility:
Public.

Description

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.

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -2 lines) Patch
Lib/test/test_exceptions.py View 3 chunks +29 lines, -2 lines 6 comments Download
Python/ceval.c View 1 chunk +13 lines, -0 lines 5 comments Download

Messages

Total messages: 5
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 ...
18 years, 1 month ago (2008-05-02 22:17:07 UTC) #1
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.
18 years, 1 month ago (2008-05-02 23:19:15 UTC) #2
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 ...
18 years, 1 month ago (2008-05-03 19:20:07 UTC) #3
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. ...
18 years, 1 month ago (2008-05-06 12:59:48 UTC) #4
Antoine Pitrou
18 years, 1 month ago (2008-05-06 15:19:41 UTC) #5
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b