Closed Bug 1020420 Opened 6 years ago Closed 6 years ago

Latin1 strings: support parseInt and parseFloat

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(4 files)

No description provided.
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.
Attachment #8434271 - Flags: review?(n.nethercote)
Attachment #8434327 - Flags: review?(n.nethercote)
Make parseFloat work with Latin1 strings.
Attachment #8434334 - Flags: review?(n.nethercote)
Let's do StringToNumber too to finish jsnum.cpp
Attachment #8434369 - Flags: review?(n.nethercote)
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.
Attachment #8434271 - Flags: review?(n.nethercote) → review+
Attachment #8434327 - Flags: review?(n.nethercote) → review+
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.
Attachment #8434334 - Flags: review?(n.nethercote) → review+
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);
Attachment #8434369 - Flags: review?(n.nethercote) → review+
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.
Depends on: 1037889
You need to log in before you can comment on or make changes to this bug.