Bug 199163 - toExponential, toFixed, and toPrecision should allow arguments up to 100
Summary: toExponential, toFixed, and toPrecision should allow arguments up to 100
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Minor
Assignee: Nobody
URL: https://github.com/tc39/ecma262/pull/857
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-24 09:27 PDT by Alexey Shvayka
Modified: 2019-09-26 11:18 PDT (History)
13 users (show)

See Also:


Attachments
Patch (40.18 KB, patch)
2019-06-24 09:52 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (40.80 KB, patch)
2019-09-24 07:03 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (42.85 KB, patch)
2019-09-24 15:26 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (42.84 KB, patch)
2019-09-26 09:52 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Shvayka 2019-06-24 09:27:55 PDT
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.
Comment 1 Alexey Shvayka 2019-06-24 09:52:36 PDT
Created attachment 372768 [details]
Patch
Comment 2 Build Bot 2019-06-24 09:54:58 PDT
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.
Comment 3 Alexey Shvayka 2019-06-26 09:27:15 PDT
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?
Comment 4 Ross Kirsling 2019-09-23 15:29:31 PDT
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.
Comment 5 Ross Kirsling 2019-09-23 16:11:37 PDT
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.
Comment 6 Alexey Shvayka 2019-09-24 07:03:29 PDT
Created attachment 379449 [details]
Patch

Revert ChakraCore test changes, tweak constants, and improve ChangeLog.
Comment 7 Ross Kirsling 2019-09-24 11:56:02 PDT
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
Comment 8 Alexey Shvayka 2019-09-24 15:26:55 PDT
Created attachment 379502 [details]
Patch

Adjust WTFString/StringNumberFixedWidth test.
Comment 9 Ross Kirsling 2019-09-24 15:38:51 PDT
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.
Comment 10 Alexey Shvayka 2019-09-26 09:52:52 PDT
Created attachment 379651 [details]
Patch

Set reviewer and remove e30 test cases.
Comment 11 WebKit Commit Bot 2019-09-26 11:17:59 PDT
Comment on attachment 379651 [details]
Patch

Clearing flags on attachment: 379651

Committed r250389: <https://trac.webkit.org/changeset/250389>
Comment 12 WebKit Commit Bot 2019-09-26 11:18:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2019-09-26 11:18:19 PDT
<rdar://problem/55751105>