| 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) *  |
Date: 2009-01-09 05:17 |
| Sounds like a fine change to me. |
|
|
| msg79456 - (view) |
Author: Raymond Hettinger (rhettinger) * |
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) * |
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) * |
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) * |
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 wrong. 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) * |
Date: 2014-09-25 19:32 |
| This has been fixed as part of issue 16191: https://hg.python.org/cpython/rev/ac30a1b1cf17#l1.2780 |
|
|