Closed
Bug 663338
Opened 13 years ago
Closed 13 years ago
parseInt fast-path should not handle very small numbers
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: jandem, Assigned: evilpie)
References
Details
Attachments
(1 file)
4.40 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
Bug 653153 fixed the fast-path for very large numbers but very small numbers have the same problem: -- js> parseInt(5e-100) 0 js> parseInt(String(5e-100)) 5 -- Spec nitpicking, but I wouldn't be surprised if this is added to test262 some day. And these bugs make it harder to find more serious bugs/regressions.
Assignee | ||
Updated•13 years ago
|
Assignee: general → evilpies
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #578851 -
Flags: review?(jdemooij)
Reporter | ||
Comment 2•13 years ago
|
||
Comment on attachment 578851 [details] [diff] [review] v1 Review of attachment 578851 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsnum.cpp @@ +428,5 @@ > + } > + if (-1.0e21 < d && d < -0.000001) { > + args.rval().setNumber(-floor(-d)); > + return true; > + } Nice, this (and the rest of the function) is a lot more readable now. Two comments: 1) Use 1.0e-6 and -1.0e-6 here (and 1e6 in the comment) to match the -1.0e21 and 1.0e21 (and people don't have to count the number of zeros). 2) For performance reasons, some functions use setDouble instead of setNumber, so the first argument may be 0.0. Please add something like this: if (d == 0.0) { args.rval().setInt32(0); return true; }
Attachment #578851 -
Flags: review?(jdemooij) → review+
Comment 3•13 years ago
|
||
Comment on attachment 578851 [details] [diff] [review] v1 Review of attachment 578851 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsnum.cpp @@ +428,5 @@ > + } > + if (-1.0e21 < d && d < -0.000001) { > + args.rval().setNumber(-floor(-d)); > + return true; > + } Doesn't setNumber(d) canonicalize 0.0d into setInt32(0), and more generally any int32 into a setInt32()? I'd thought setDouble always set double, setInt32 always set int32, and setNumber tested and chose int32 iff possible, but maybe my memory's hazy.
Assignee | ||
Comment 4•13 years ago
|
||
This is right, but some code uses setDouble, which could produce 0, eg. Math.asin.
Assignee | ||
Comment 5•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/41e3dd30ad8f
Comment 6•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/41e3dd30ad8f
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in
before you can comment on or make changes to this bug.
Description
•