Title: misplaced (or misleading) assert in ceval.c
Type: enhancement Stage: resolved
Components: Versions: Python 3.4
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: berker.peksag, jimjjewett, jyasskin, rhettinger
Priority: normal Keywords: patch

Created on 2009-01-09 04:52 by skip.montanaro, last changed 2014-09-25 19:32 by berker.peksag. This issue is now closed.

File name Uploaded Description Edit
assert.diff skip.montanaro, 2009-01-09 04:52 review
Messages (8)
msg79453 - (view) Author: Skip Montanaro (skip.montanaro) * Date: 2009-01-09 04:52
There is what I believe is a misplaced - or at least misleading - assert
in the while loop following the fast_block_end label.  If why != WHY_YIELD
before the loop starts I don't see how that relationship could change
within the loop.  Proposed patch against py3k branch attached.

(Yes, I realize this is trivial and that the assert is compiled away in
non-debug builds.)
msg79454 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2009-01-09 05:17
Sounds like a fine change to me.
msg79456 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2009-01-09 07:23
IIRC, I think I put this here a long while ago when replacing or
removing a corresponding if-test that had always been compiled-in.  The
assertion was a way of validating that the refactoring and elimination
of tests had not broken anything.  If that was the case, I prefer to
leave this in, perhaps with some comment.  The control flow and known
conditions at each step are a bit hairy in this part of the code.
msg79475 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2009-01-09 15:51
I don't see why the refactoring has to maintain the same logic bug as
the original.  I'm with Skip & Jeffrey.
msg79497 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2009-01-09 19:56
I wasn't opposing the patch.  Just wanted to look back at why the
assertion was put there in the first place.  If you want it in, go ahead.
msg79749 - (view) Author: Jim Jewett (jimjjewett) Date: 2009-01-13 17:19
I agree with Raymond.  A comment *might* be sufficient, but ... in some 
sense, that is the purpose of an assert.

The loop is reasonably long; it already includes macros which could 
(but currently don't) change the value, and function calls which might 
plausibly (but don't) reset a "why" variable.  The why variable is 
techically local, but the scope is still pretty large, so that isn't 
clear at first.

It took me some work to verify the assertion, and I'm not at all 
confident that a later change wouldn't violate it.  Nor am I confident 
that the symptoms would make for straightforward debugging.  (Would it 
look like stack corruption?  Would it take several more opcodes before 
a problem was visible?)
msg79756 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2009-01-13 17:58
The assert seems confusing to me because it's overly specific. It causes
me to ask, "what's special about WHY_YIELD that why might be set to it?"
If I understand the loop correctly, we could rewrite the top as:

assert(why != WHY_YIELD);  /* These two values aren't handled in the
loop. */
assert(why != WHY_RERAISE);
orig_why = why;
while (why != WHY_NOT && f->f_iblock > 0) {
    /* The code does not change 'why' without breaking out of the loop. */
    assert(why == orig_why);

which would tell the reader more about the state of the world without
focusing their attention on anything that isn't somehow special.

Of course, nothing prevents the code from changing orig_why (*pines for
const and late declarations*), but such a change would be more obviously

Was there another reason to assert(why!=WHY_YIELD) than that the if's
don't handle it? Is 'why' more likely to become WHY_YIELD than WHY_RERAISE?
msg227568 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2014-09-25 19:32
This has been fixed as part of issue 16191:
Date User Action Args
2014-09-25 19:32:04berker.peksagsetstatus: open -> closed

nosy: + berker.peksag
messages: + msg227568

resolution: out of date
stage: patch review -> resolved
2013-10-14 02:57:03gvanrossumsetnosy: - gvanrossum
2013-10-14 02:12:51ezio.melottisetstage: patch review
versions: + Python 3.4, - Python 3.2
2010-07-11 02:19:59terry.reedysetversions: + Python 3.2, - Python 2.6, Python 3.0, Python 3.1, Python 2.7
2010-05-20 20:34:58skip.montanarosetnosy: - skip.montanaro
2009-01-13 17:58:46jyasskinsetmessages: + msg79756
2009-01-13 17:19:08jimjjewettsetnosy: + jimjjewett
messages: + msg79749
2009-01-09 19:56:00rhettingersetassignee: rhettinger ->
messages: + msg79497
2009-01-09 15:51:53gvanrossumsetnosy: + gvanrossum
messages: + msg79475
2009-01-09 07:23:42rhettingersetassignee: rhettinger
messages: + msg79456
nosy: + rhettinger
2009-01-09 05:17:33jyasskinsetnosy: + jyasskin
messages: + msg79454
2009-01-09 04:52:13skip.montanarocreate