1020420 - Latin1 strings: support parseInt and parseFloat

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - parseInt

Luke reviewed most patches so far, but I'm going to spread them out a bit to avoid overloading him :)

Nicholas, my strategy has been the following for most functions: (1) Factor out the part of the function that works on chars (2) templatize it to work on both char types (3) write tests. This has been working pretty well so far.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Refactor js_strtod

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - parseFloat

Make parseFloat work with Latin1 strings.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 4 - StringToNumber

Let's do StringToNumber too to finish jsnum.cpp

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 8434271 [details] [diff] [review]
Part 1 - parseInt

Review of attachment 8434271 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/latin1/parseInt-parseFloat.js
@@ +3,5 @@
> +    assertEq(parseInt(toLatin1("12345abc")), 12345);
> +    assertEq(parseInt(toLatin1("0x5")), 0x5);
> +    assertEq(parseInt(toLatin1("-123")), -123);
> +    assertEq(parseInt(toLatin1("xyz")), NaN);
> +    assertEq(parseInt(toLatin1("1234GHI"), 17), 94298);

My base-17 arithmetic is rusty, but this appears to be correct :)

@@ +8,5 @@
> +    assertEq(parseInt(toLatin1("9007199254749999")), 9007199254750000);
> +    assertEq(parseInt(toLatin1("9007199254749998"), 16), 10378291982571444000);
> +
> +    // TwoByte
> +    assertEq(parseInt("12345abc\u1200"), 12345);

You could do this instead:

> assertEq(parseInt(toLatin1("12345abc")), parseInt("12345abc\u1200"));

I don't mind either way, though.

::: js/src/jsnum.cpp
@@ +438,5 @@
>                  stripPrefix = false;
>          }
>      }
>  
> +    size_t length = inputString->length();

Move this line after the |!linear| check below.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8434334 [details] [diff] [review]
Part 3 - parseFloat

Review of attachment 8434334 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/latin1/parseInt-parseFloat.js
@@ +28,5 @@
> +    assertEq(parseFloat(toLatin1("-Infinity")), -Infinity);
> +    assertEq(parseFloat(toLatin1("+Infinity")), Infinity);
> +
> +    // TwoByte
> +    assertEq(parseFloat("3.1415\u0FFF"), 3.1415);

As before, you could compare the Latin1 results to the equivalent TwoByte results.

Also, it would be good to have at least one of these tests have leading whitespace.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8434369 [details] [diff] [review]
Part 4 - StringToNumber

Review of attachment 8434369 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsnum.cpp
@@ +1578,1 @@
>      return CharsToNumber(cx, inspector.twoByteChars(), str->length(), result);

I would write this like so:

> return inspector.hasLatin1Chars()
>        ? CharsToNumber(cx, inspector.latin1Chars(), str->length(), result)
>        : CharsToNumber(cx, inspector.twoByteChars(), str->length(), result);

Comment 8

[Jan de Mooij [:jandem]]

Thanks for the quick reviews!

https://hg.mozilla.org/integration/mozilla-inbound/rev/32b157ae5ed7
https://hg.mozilla.org/integration/mozilla-inbound/rev/1962fd3aa819
https://hg.mozilla.org/integration/mozilla-inbound/rev/654117be57c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7061043b85

(In reply to Nicholas Nethercote [:njn] from comment #5)
> You could do this instead:
> 
> > assertEq(parseInt(toLatin1("12345abc")), parseInt("12345abc\u1200"));
> 
> I don't mind either way, though.

What I like about comparing the number result explicitly is that if I somehow break the algorithm completely (in the same way for both Latin1 and TwoByte), the test will still fail. But yeah, we have other tests to catch that... I'll keep this in mind for other tests :)

> Move this line after the |!linear| check below.

Done.

(In reply to Nicholas Nethercote [:njn] from comment #6)
> Also, it would be good to have at least one of these tests have leading
> whitespace.

Good idea; added leading whitespace to some of them.

(In reply to Nicholas Nethercote [:njn] from comment #7)
> I would write this like so:
> 
> > return inspector.hasLatin1Chars()
> >        ? CharsToNumber(cx, inspector.latin1Chars(), str->length(), result)
> >        : CharsToNumber(cx, inspector.twoByteChars(), str->length(), result);

Yes that looks nice, I'll start doing that in other patches too.

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/32b157ae5ed7
https://hg.mozilla.org/mozilla-central/rev/1962fd3aa819
https://hg.mozilla.org/mozilla-central/rev/654117be57c2
https://hg.mozilla.org/mozilla-central/rev/7e7061043b85
https://hg.mozilla.org/mozilla-central/rev/e9d959645b8f