Closed
Bug 1020420
Opened 11 years ago
Closed 11 years ago
Latin1 strings: support parseInt and parseFloat
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(4 files)
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
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)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #8434327 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 3•11 years ago
|
||
Make parseFloat work with Latin1 strings.
Attachment #8434334 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 4•11 years ago
|
||
Let's do StringToNumber too to finish jsnum.cpp
Attachment #8434369 -
Flags: review?(n.nethercote)
Comment 5•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #8434327 -
Flags: review?(n.nethercote) → review+
Comment 6•11 years ago
|
||
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 7•11 years ago
|
||
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+
Assignee | ||
Comment 8•11 years ago
|
||
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•11 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•