Bug 199163 – toExponential, toFixed, and toPrecision should allow arguments up to 100

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Alexey Shvayka]

ECMA262:
  https://tc39.es/ecma262/#sec-number.prototype.toexponential (step 8)
  https://tc39.es/ecma262/#sec-number.prototype.tofixed (step 4)
  https://tc39.es/ecma262/#sec-number.prototype.toprecision (step 8)

ES5 also allowed those arguments to exceed 20, but in informal note, while normative text was:

```
15.7.4.5 Number.prototype.toFixed (fractionDigits)

2. If f < 0 or f > 20, throw a RangeError exception.
```

V8 and SpiderMonkey accept arguments up to 100.

[Alexey Shvayka]

Created attachment 372768 [details]
Patch

[EWS Watchlist]

Attachment 372768 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/dtoa/double-conversion.h:42:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Shvayka]

Style check failure is not related to this change: there are ~56 occurrences of class access modifiers, indented by 1 space in Source/WTF:

```
class DoubleToStringConverter {
 public:
  static const int kMaxFixedDigitsBeforePoint = 60;
```

Should I ignore it or unindent all of them or only in changed file?

[Ross Kirsling]

Comment on attachment 372768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372768&action=review

Ha, I just did a fix patch for this myself and then realized that there's already one here...
I suppose I'll just add comments to yours instead.

> JSTests/ChakraCore/test/Number/toString.js:-31
> +    writeLine("n.toFixed(90):  " + n.toFixed(90));
>      safeCall(function () { n.toFixed(-1); });
> -    safeCall(function () { n.toFixed(21); });

It's probably best not to modify a ChakraCore test (especially since these lines haven't changed upstream) -- let's just update the output.

> Source/JavaScriptCore/ChangeLog:8
> +        getIntegerArgumentInRange had quite a few arguments and wasn't easy to follow, so I inlined it in methods.

Seems slightly unnecessary but I agree that the result is easier to read. :)

> Source/WTF/wtf/dtoa.h:30
> -using NumberToStringBuffer = std::array<char, 96>;
> +using NumberToStringBuffer = std::array<char, 176>;

This can be just 123 (<21 digits> + decimal point + <100 digits> + '\0'). Only toFixed actually uses this many.

> Source/WTF/wtf/dtoa/double-conversion.h:42
> -  static const int kMaxFixedDigitsAfterPoint = 60;
> +  static const int kMaxFixedDigitsAfterPoint = 100;

This is the only line that *needs* to change, but it'd be nice to update the others to their correct values too, i.e.:

  static const int kMaxFixedDigitsBeforePoint = 21;
  static const int kMaxFixedDigitsAfterPoint = 100;

  static const int kMaxExponentialDigits = 100;

  static const int kMinPrecisionDigits = 1;
  static const int kMaxPrecisionDigits = 100;

(And accordingly, lines 208 and 209 of double-conversion.cc.)

...that said, given that this is taken from https://github.com/google/double-conversion, we also might want to minimize changes...hmm.

One way or another, I believe it's okay to ignore style errors for this file.

[Ross Kirsling]

Oh also, for the ChangeLog, it might be helpful to mention that it was changed for ES2018 in https://github.com/tc39/ecma262/pull/857.

[Alexey Shvayka]

Created attachment 379449 [details]
Patch

Revert ChakraCore test changes, tweak constants, and improve ChangeLog.

[Ross Kirsling]

Hmm, looks like lowering kMaxFixedDigitsBeforePoint requires us to deal with the e21 and e30 cases here too:
https://github.com/WebKit/webkit/blob/master/Tools/TestWebKitAPI/Tests/WTF/WTFString.cpp#L159-L169

[Alexey Shvayka]

Created attachment 379502 [details]
Patch

Adjust WTFString/StringNumberFixedWidth test.

[Ross Kirsling]

Comment on attachment 379502 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379502&action=review

LGTM pending EWS.

> Tools/TestWebKitAPI/Tests/WTF/WTFString.cpp:163
> -    EXPECT_STREQ("1000000000000000000000.000000", testStringNumberFixedWidth(1e21));
> -    EXPECT_STREQ("1000000000000000019884624838656.000000", testStringNumberFixedWidth(1e30));
> +    EXPECT_STREQ("", testStringNumberFixedWidth(1e21));
> +    EXPECT_STREQ("", testStringNumberFixedWidth(1e30));

I suppose the e30 cases could just be removed.

[Alexey Shvayka]

Created attachment 379651 [details]
Patch

Set reviewer and remove e30 test cases.

[WebKit Commit Bot]

Comment on attachment 379651 [details]
Patch

Clearing flags on attachment: 379651

Committed r250389: <https://trac.webkit.org/changeset/250389>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55751105>