Last Comment Bug 663338 - parseInt fast-path should not handle very small numbers
: parseInt fast-path should not handle very small numbers
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Tom Schuster [:evilpie]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: es5
  Show dependency treegraph
 
Reported: 2011-06-10 02:05 PDT by Jan de Mooij [:jandem]
Modified: 2012-02-01 13:57 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (4.40 KB, patch)
2011-12-03 11:06 PST, Tom Schuster [:evilpie]
jdemooij: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2011-06-10 02:05:06 PDT
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.
Comment 1 Tom Schuster [:evilpie] 2011-12-03 11:06:55 PST
Created attachment 578851 [details] [diff] [review]
v1
Comment 2 Jan de Mooij [:jandem] 2011-12-05 08:23:51 PST
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;
}
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-05 12:58:57 PST
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.
Comment 4 Tom Schuster [:evilpie] 2011-12-05 13:03:06 PST
This is right, but some code uses setDouble, which could produce 0, eg. Math.asin.
Comment 6 Ed Morley [:emorley] 2011-12-07 02:49:46 PST
https://hg.mozilla.org/mozilla-central/rev/41e3dd30ad8f

Note You need to log in before you can comment on or make changes to this bug.