Do not allocate NaN and Infinity strings in num_toFixed_impl

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: rajindery, Assigned: rajindery, Mentored)

Tracking

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

We allocate the strings here, however we should use pre-allocated atoms.

https://dxr.mozilla.org/mozilla-central/rev/214884d507ee369c1cf14edb26527c4f9a97bf48/js/src/dtoa.c#2662-2664
Assignee: nobody → rajindery
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Posted patch bug1286114.patch (obsolete) — Splinter Review
Please review.
Attachment #8770044 - Flags: review?(arai.unmht)
We should just add NegativeInfinity (-0) to CommonPropertyNames instead of using StringBuilder.
Tom agreed, I should have suggested this :P
Comment on attachment 8770044 [details] [diff] [review]
bug1286114.patch

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

Thanks :)
Please address the following comments, and ask review again.

::: js/src/jsnum.cpp
@@ +916,5 @@
>  {
>      MOZ_ASSERT(IsNumber(args.thisv()));
>  
> +    // If 'this' (callee) value is -Infinity, Infinity or NaN
> +    // return String representation.

This comment is redundant. please remove.

@@ +917,5 @@
>      MOZ_ASSERT(IsNumber(args.thisv()));
>  
> +    // If 'this' (callee) value is -Infinity, Infinity or NaN
> +    // return String representation.
> +    double d = Extract(args.thisv());

Just like toExponential, can you add spec step comments?

@@ +919,5 @@
> +    // If 'this' (callee) value is -Infinity, Infinity or NaN
> +    // return String representation.
> +    double d = Extract(args.thisv());
> +
> +    if (mozilla::IsNaN(d)) {

So you'll see that this is step 4. and we should have step 2 and 3 before this.
Both are already done in this function, so move those block before this line.

Please add testcase for the execution order between them (see the comment below)

@@ +922,5 @@
> +
> +    if (mozilla::IsNaN(d)) {
> +        args.rval().setString(cx->names().NaN);
> +        return true;
> +    } else if (mozilla::IsInfinite(d)) {

Do not use "else" after return.

@@ +927,5 @@
> +        if(d > 0) {
> +            args.rval().setString(cx->names().Infinity);
> +            return true;
> +        }
> +        StringBuffer sb(cx);

As evilpie says, please change negative infinity case to return cx->names().NegativeInfinity, and add NegativeInfinity to js/src/vm/CommonPropertyNames.h.

Also, apply the same change to toExponential and toPrecision.


Actually, I was about to ask merge those 3 code for NaN/Infinity handling (as they're exactly same), but if we can return atom directly, the code should be simple enough and we won't have to merge them.

::: js/src/tests/ecma_6/Number/ToNumber.js
@@ +18,5 @@
>  assertEq(+"0o66", 54);
>  
> +assertEq(Number.prototype.toFixed.call(-Infinity), "-Infinity");
> +assertEq(Number.prototype.toFixed.call(Infinity), "Infinity");
> +assertEq(Number.prototype.toFixed.call(NaN), "NaN");

This file is a testcase for Number constructor.
Can you create a new dedicated file for toFixed?

Also, please check passing out-of-range fractionDigits throws RangeError even with NaN and Infinity (this is different behavior than toExponential and toPrecision...), with assertThrowsInstanceOf.
Attachment #8770044 - Flags: review?(arai.unmht) → feedback+
(In reply to Tom Schuster [:evilpie] from comment #2)
> We should just add NegativeInfinity (-0) to CommonPropertyNames instead of
> using StringBuilder.

Thank you for pointing that out :)
Posted patch bug1286114.patch (obsolete) — Splinter Review
Feedback addressed, please review.
Attachment #8770044 - Attachment is obsolete: true
Attachment #8770072 - Flags: review?(arai.unmht)
Comment on attachment 8770072 [details] [diff] [review]
bug1286114.patch

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

Thank you again :)
Almost there, please address the following comments, mostly cosmetic changes.

::: js/src/jsnum.cpp
@@ +909,5 @@
>  
>  /*
>   * In the following three implementations, we allow a larger range of precision
>   * than ECMA requires; this is permitted by ECMA-262.
>   */

Please add the following comment here, so that "step N" makes sense in function body.

// ES 2017 draft rev f8a9be8ea4bd97237d176907a1e3080dce20c68f 20.1.3.3.

@@ +936,5 @@
> +        args.rval().setString(cx->names().NaN);
> +        return true;
> +    }
> +
> +    // Steps 5-6.

in toFixed, Infinity is handled in Steps 5-7, 9, and no special case is defined in the spec.

so it would be better changing the comment to

  // Steps 5-7, 9 (optimized path for Infinity).

or something like that.

(I'm wondering why there are so much differences in the spec...)

@@ +947,5 @@
> +        args.rval().setString(cx->names().NegativeInfinity);
> +        return true;
> +    }
> +
> +    // Steps 7-9.

Also, steps 5-6 are also handled here, for normal number, so please change it to "// Steps 5-9."

I overlooked this for toExponential and toPrecision, sorry.  can you fix the step numbers there too?

@@ +996,4 @@
>          return true;
>      }
>  
>      // Steps 8-15.

So, here, "// Steps 5-6, 8-15."

@@ +1054,4 @@
>          return true;
>      }
>  
>      // Steps 8-14.

and "// Steps 5-6, 8-14." here.
Attachment #8770072 - Flags: review?(arai.unmht) → feedback+
Feedback applied, please review.
Attachment #8770072 - Attachment is obsolete: true
Attachment #8770087 - Flags: review?(arai.unmht)
Comment on attachment 8770087 [details] [diff] [review]
bug1286114.patch

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

Thank you!
Attachment #8770087 - Flags: review?(arai.unmht) → review+
https://hg.mozilla.org/mozilla-central/rev/28f796a18c0c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.