NumberFormat: default mnsd value is 1

NumberFormat: default mnsd value is 1

CreateNumberFormat of src/js/i18n.js implements
http://www.ecma-international.org/ecma-402/1.0/#sec-11.1.1.1, but has a typo
in step 33a. The spec says that the default value for minimumSignificantDigits
should be 1, while the script set it to 0.

This CL fixes that typo and adds a test for that.

BUG=v8:5554
Review-Url: https://codereview.chromium.org/2694673003
Cr-Commit-Position: refs/heads/master@{#43197}
Committed: https://chromium.googlesource.com/v8/v8/+/0559fd7082fa9c12efd081e3e74fb34ecf6158ca

[vabr (Chromium), 2017-02-13 22:07:07]

Description was changed from

==========
NumberFormat: default mnsd lower bound is 1

CreateNumberFormat of src/js/i18n.js implements
http://www.ecma-international.org/ecma-402/1.0/#sec-11.1.1.1, but has a typo
in step 33a. The spec says that the lower bound for minimumSignificantDigits
should be 1, while the script set it to 0.

This CL fixes that typo and adds a test for that.

BUG=v8:5554
==========

to

==========
NumberFormat: default mnsd value is 1

CreateNumberFormat of src/js/i18n.js implements
http://www.ecma-international.org/ecma-402/1.0/#sec-11.1.1.1, but has a typo
in step 33a. The spec says that the default value for minimumSignificantDigits
should be 1, while the script set it to 0.

This CL fixes that typo and adds a test for that.

BUG=v8:5554
==========

[vabr (Chromium), 2017-02-13 22:07:13]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 22:07:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 22:30:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 22:30:52]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2017-02-13 22:31:31]

vabr@chromium.org changed reviewers:
+ littledan@chromium.org

[vabr (Chromium), 2017-02-13 22:31:32]

Hello Dan,

Would you mind also reviewing this one?

Thanks,
Vaclav

[Dan Ehrenberg, 2017-02-14 13:43:46]

lgtm

For bonus points, you could make a test262 test verifying the fix so that all JS
implementations can share this regression test. test262 tests can be developed
and checked in locally for a patch in test/test262/local-tests, if you prefer,
using a process documented here:
https://docs.google.com/document/d/16bj7AIDgZLv4WOsUEzQ5NzcEN9_xo095e88Pz8FC5...
. (Or, you could just make a PR against upstream test262 directly.) If you have
trouble writing test262 tests, I'd be interested in addressing the problems, but
anyway, we are not writing test262 tests for all patches at the moment, so this
is all optional.

[Dan Ehrenberg, 2017-02-14 13:44:37]

cc jshin if he is interested in taking a look, but this seems like a simple
typo, and the fact that SpiderMonkey and JSC do the spec'd behavior makes me not
so worried about compat.

[Dan Ehrenberg, 2017-02-14 13:46:23]

By the way, the most relevant spec text to look at for implementing ECMA 402 is
the current draft spec, which you can find at https://tc39.github.io/ecma402/ .
Although this part is the same in the draft spec as in v1, other things have
changed over time.

[vabr (Chromium), 2017-02-14 17:11:17]

Thanks, Dan, both for the review, and for the useful information.

I'm happy to write the test262 test, and will try the approach with creating the
patch locally in the V8 checkout. I will lend this CL as it is, and once I'm
done with the test262, I will consult you for whether it should replace the test
added in test/intl/number-format/check-digit-ranges.js.

Also thanks for the pointer to the current draft spec. The paragraph numbers in
the names of the test262 tests seem to refer the
http://www.ecma-international.org/ecma-402/1.0, correct?

Cheers,
Vaclav

[vabr (Chromium), 2017-02-14 17:11:27]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2017-02-14 17:11:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 17:12:50]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1487092287725050, "parent_rev":
"8ec17c7bb5ee10809f42dc8b07b1cdc2c54ad91e", "commit_rev":
"0559fd7082fa9c12efd081e3e74fb34ecf6158ca"}

[commit-bot: I haz the power, 2017-02-14 17:12:55]

Description was changed from

==========
NumberFormat: default mnsd value is 1

CreateNumberFormat of src/js/i18n.js implements
http://www.ecma-international.org/ecma-402/1.0/#sec-11.1.1.1, but has a typo
in step 33a. The spec says that the default value for minimumSignificantDigits
should be 1, while the script set it to 0.

This CL fixes that typo and adds a test for that.

BUG=v8:5554
==========

to

==========
NumberFormat: default mnsd value is 1

CreateNumberFormat of src/js/i18n.js implements
http://www.ecma-international.org/ecma-402/1.0/#sec-11.1.1.1, but has a typo
in step 33a. The spec says that the default value for minimumSignificantDigits
should be 1, while the script set it to 0.

This CL fixes that typo and adds a test for that.

BUG=v8:5554

Review-Url: https://codereview.chromium.org/2694673003
Cr-Commit-Position: refs/heads/master@{#43197}
Committed:
https://chromium.googlesource.com/v8/v8/+/0559fd7082fa9c12efd081e3e74fb34ecf6...
==========

[commit-bot: I haz the power, 2017-02-14 17:12:55]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/v8/v8/+/0559fd7082fa9c12efd081e3e74fb34ecf6...

[Dan Ehrenberg, 2017-02-14 17:27:08]

On 2017/02/14 17:11:17, vabr (OOO until late Feb) wrote:
> Thanks, Dan, both for the review, and for the useful information.
> 
> I'm happy to write the test262 test, and will try the approach with creating
the
> patch locally in the V8 checkout. I will lend this CL as it is, and once I'm
> done with the test262, I will consult you for whether it should replace the
test
> added in test/intl/number-format/check-digit-ranges.js.
> 
> Also thanks for the pointer to the current draft spec. The paragraph numbers
in
> the names of the test262 tests seem to refer the
> http://www.ecma-international.org/ecma-402/1.0, correct?
> 
> Cheers,
> Vaclav

Paragraph numbers in the names of test262 tests is something that was done in
the past, but is not the current practice for new tests, whose naming convention
is documented at https://github.com/tc39/test262/blob/master/CONTRIBUTING.md .
In general, I believe tests with old-style names might refer to a range of
different specs, but the Intl ones probably refer to that document, though some
might have been updated to the v2 spec at
http://www.ecma-international.org/ecma-402/2.0/ (the contents were definitely
updated; I'm not sure whether the names were). The tests are generally testing
for the top-of-tree behavior, which is at https://tc39.github.io/ecma402/ .

Anyway, for this, I think it'd be reasonable to make a new test file rather than
squeeze it into an existing file. Generally, test262 tries to keep files
relatively small and focused.