350559 - js1_7/geniter/throw-after-close.js|js1_7/geniter/throw-forever.js, result: FAILED

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

regression since 20060827 trunk. bug 350312?

js1_7/geniter/throw-after-close.js
 Expected value 'false', Actual value 'it.throw("foobar") failed

js1_7/geniter/throw-forever.js
Expected value 'false', Actual value 'it.throw("baz") failed'

Comment 1

[Brendan Eich [:brendan]]

This is a regression from the patch for bug 349320.

PEP 342 is unclear about what throw on a closed generator should do:

    3. Add a new throw() method for generator-iterators, which raises
       an exception at the point where the generator was paused, and
       which returns the next value yielded by the generator, raising
       StopIteration if the generator exits without yielding another
       value.  (If the generator does not catch the passed-in exception,
       or raises a different exception, then that exception propagates
       to the caller.)

Nevertheless, the JS implementation before rev 3.35 of jsiter.c, and Python 2.5c1, both let the thrown exception propagate, rather than raising StopIteration.

/be

Comment 2

[Igor Bukanov]

Attachment: Fix

There are no reasons not be compatible with Python.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 235930 [details] [diff] [review]
Fix

Simple and direct -- only nit is the default: case with assertion that op is JSGENOP_CLOSE -- with a disjoint sum type like JSGeneratorOp, unless we expect to add more ops, covering all cases with switch seems better (GCC will warn if someone adds an enumerator and forgets to extend the switch).

/be

Comment 4

[Brendan Eich [:brendan]]

Regression fix needed on branch.

/be

Comment 5

[Igor Bukanov]

(In reply to comment #3)
> (From update of attachment 235930 [details] [diff] [review] [edit])
> Simple and direct -- only nit is the default: case with assertion that op is
> JSGENOP_CLOSE -- with a disjoint sum type like JSGeneratorOp, unless we expect
> to add more ops, covering all cases with switch seems better (GCC will warn if
> someone adds an enumerator and forgets to extend the switch).

In this switch it does not matter, but try to replace "default with assert" in the outer switch and GCC starts to complain about not initialized wasNewborn variable :(. Using default+assert is less ugly for my eyes then #ifdef __GNUC__ (especially taking into account that GNUC when read as transliterated into latin  Russian sounds like "vile" or "foul" for my ears ;). Then for consistency it make sense to use default+assert in all 3 switches across the function.

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> (In reply to comment #3)
> In this switch it does not matter, but try to replace "default with assert" in
> the outer switch and GCC starts to complain about not initialized wasNewborn
> variable :(.

GCC is weak that way.

> Using default+assert is less ugly for my eyes then #ifdef __GNUC__
> (especially taking into account that GNUC when read as transliterated into
> latin  Russian sounds like "vile" or "foul" for my ears ;).

I had no idea -- that's karmic backlash, right there ;-)

> Then for
> consistency it make sense to use default+assert in all 3 switches across the
> function.

Cool -- nit retracted.

/be

Comment 7

[Igor Bukanov]

I committed the patch from comment 2 to the trunk:

Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.37; previous revision: 3.36
done

Comment 8

[Igor Bukanov]

(In reply to comment #6)
> > Using default+assert is less ugly for my eyes then #ifdef __GNUC__
> > (especially taking into account that GNUC when read as transliterated into
> > latin  Russian sounds like "vile" or "foul" for my ears ;).
> 
> I had no idea -- that's karmic backlash, right there ;-)

There are a few jokes in Russian with that word game GNU(s|c)<->vile, like "vile soft showed it quality".

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 235930 [details] [diff] [review]
Fix

a=beltzner for the 181drivers

Comment 10

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060830 windows/mac*/linux

Comment 11

[Igor Bukanov]

I committed the patch from comment 2 to MOZILLA_1_8_BRANCH:

Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.17.2.13; previous revision: 3.17.2.12
done

Comment 12

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20060831 windows/mac*/linux