Incorrect result with number.toExponential(undefined)

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla28
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

js> (123456789).toExponential()
"1.23456789e+8"
js> (123456789).toExponential(undefined)
"1e+8"

The second value is incorrect per ES5 15.7.4.6.
Whiteboard: [good first bug]
I was going to explain why this is a [good first bug], but it's a one-line change so it's faster to just fix it myself. Patch tomorrow.
Assignee: general → jandemooij
Status: NEW → ASSIGNED
Whiteboard: [good first bug]
Jandem, I'm pretty sure you won't know what that one-line fix is, anymore, but maybe you can find out, again?
Flags: needinfo?(jdemooij)
Simply change `if (args.length() == 0)` to `if (!args.hasDefined(0))` in num_toExponential_impl.

For bonus points improve overall spec compliance, e.g. `Infinity.toExponential(Infinity)` should return Infinity instead of throwing a RangeError. And remove dead code in num_toPrecision_impl, the `if (args.length() == 0){...}` branch will never be true.
Thanks, that's very helpful!

André, don't you sometimes get an itch, you know, where you just *know* you want to write a patch? ;)
Posted patch PatchSplinter Review
Not exactly "tomorrow", but here's a patch. This also removes the dead branch André noticed in num_toPrecision_impl.

> For bonus points improve overall spec compliance, e.g.
> `Infinity.toExponential(Infinity)` should return Infinity instead of
> throwing a RangeError.

That's not as trivial and I don't really have the time to fix this. But I can file a follow-up bug for it.
Attachment #8340004 - Flags: review?(till)
Flags: needinfo?(jdemooij)
Comment on attachment 8340004 [details] [diff] [review]
Patch

Review of attachment 8340004 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Not that this particular test really contains anything copyrightable, but I *do* like the idea of having license headers in all test files. Ideally just Public Domain, IMO.
Attachment #8340004 - Flags: review?(till) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b58f32ae7646

(In reply to Till Schneidereit [:till] from comment #6)
> Not that this particular test really contains anything copyrightable, but I
> *do* like the idea of having license headers in all test files. Ideally just
> Public Domain, IMO.

We add a ton of jit-tests, a lot of them from fuzzers, and IMO it seems overkill to add license headers to all of them..
https://hg.mozilla.org/mozilla-central/rev/b58f32ae7646
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.