124339 - regexp.lastIndex: should be uint32 or integer-valued double?

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Phil Schwartau]

Note these observations by Brendan when he fixed bug 76717. 
As a result, I'm filing this bug and cc'ing Waldemar -


------- Additional Comment #18 From Brendan Eich 2002-02-07 18:55 -------

The testsuite made me transpose lastIndex and parenCount, so the latter
was the 24-bit field sharing a 32-bit word wit flags.  That seems bogus
of the testsuite, but I made it happy.  Who needs > 16M parens anyhoo?

ECMA seems to say that lastIndex is a read-write property whose value
is an Integer.  The regexp semantics won't ever let it be negative,
but put no upper bound less than Integer's u.b. on its domain.  Does that
really require conforming implementations to store lastIndex using an 
integer-valued double? We don't, and the testsuite actually expects us
to use a uint32.  I say both the engine and the suite are buggy.  Phil,
can you ping waldemar and open a new bug with fixed/new tests as needed?  
Thanks,

/be


------- Additional Comment #19 From Brendan Eich 2002-02-07 19:00 -------

The test that wants uint32 storage is ecma_2/RegExp/properties-001.js, IIRC.

/be

Comment 1

[Brendan Eich [:brendan]]

I was wrong, the relevant test is ecma_2/RegExp/properties-002.js, but it does
not test the domain of lastIndex sufficiently -- it needs to set Math.pow(2,32)
and larger values and see whether they round-trip.

Patch in a sec, it fixes a transposed-member-names-without-transposed-comments
botch in jsregexp.h that I committed at the last second.  It also aligns the
double lastIndex well on 32-bit and 64-bit platforms.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix

Looking for (new) testsuite goodness, r=, and sr= today.

/be

Comment 3

[rogerl (gone)]

Comment on attachment 68604 [details] [diff] [review]
proposed fix

r=rogrel

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 68604 [details] [diff] [review]
proposed fix

Sure.  It's just .lastIndex, right? =)

sr=shaver.

Comment 5

[Brendan Eich [:brendan]]

Fix is in.

/be

Comment 6

[Brendan Eich [:brendan]]

Fix is in, I say!

/be

Comment 7

[Phil Schwartau]

Five new cases ranging between Math.pow(2,32) and Number.MAX_VALUE
have now been added to the testcase to cover the range between the
uint32 upper bound and the upper bound for doubles. These new cases
failed as follows before the fix for this bug went in:


*-* Testcase ecma_2/RegExp/properties-002.js failed:

Failure messages were:
/\B/.lastIndex = 0 FAILED! expected: 4294967296
/\B/.lastIndex = 1 FAILED! expected: 4294967297
/\B/.lastIndex = 0 FAILED! expected: 8589934592
/\B/.lastIndex = 0 FAILED! expected: 1099511627776
/\B/.lastIndex = 0 FAILED! expected: 1.7976931348623157e+308


Now the testcase is passing in the debug/optimized JS shell
on WinNT, Linux, and Mac9.1. Marking Verified FIXED -

Comment 8

[Phil Schwartau]

I have filed bug 124508 against Rhino on this same issue -