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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: jandem, Assigned: evilpie)

References

Details

Attachments

(1 file)

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: general → evilpies
Attached patch v1Splinter Review
Attachment #578851 - Flags: review?(jdemooij)
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 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.
This is right, but some code uses setDouble, which could produce 0, eg. Math.asin.
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.

Attachment

General

Created:
Updated:
Size: