parseInt fast-path should not handle very small numbers

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jandem, Assigned: evilpie)

Tracking

unspecified
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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

6 years ago
Assignee: general → evilpies
(Assignee)

Comment 1

6 years ago
Created attachment 578851 [details] [diff] [review]
v1
Attachment #578851 - Flags: review?(jdemooij)
(Reporter)

Comment 2

6 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 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

6 years ago
This is right, but some code uses setDouble, which could produce 0, eg. Math.asin.
(Assignee)

Comment 5

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/41e3dd30ad8f
https://hg.mozilla.org/mozilla-central/rev/41e3dd30ad8f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.