Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix for the wrong test of Intl.NumberFormat (ToRawFixed) #856

Merged
merged 2 commits into from
Mar 1, 2017

Conversation

Yaffle
Copy link
Contributor

@Yaffle Yaffle commented Feb 11, 2017

Note:

  1. ToRawFixed(12344501000000000000000000000000000, 3, 1, 3) should return "12344501000000000487815444678311936.0" according to the spec. (see https://tc39.github.io/ecma402/#sec-torawfixed );
  2. 12344501000000000487815444678311936 === 12344501000000000000000000000000000 for binary64 floating points and so it is better to change the left part to avoid a confusion.
  3. (12344501000000000487815444678311936).toPrecision(21) === "1.23445010000000004878e+34" in Chrome and Firefox;

P.S. the spefication for ToRawFixed is quite demanding here, may be it should be updated to allow some inaccuracy when x >= 1e+21.

Note:
12344501000000000487815444678311936 === 12344501000000000000000000000000000 for binary64 floating points;
@Yaffle Yaffle changed the title fix for the wrong test of Intl.NumberFormat fix for the wrong test of Intl.NumberFormat (ToRawFixed) Feb 11, 2017
@littledan
Copy link
Member

Interesting find. I can't say I disagree with your reading of ToRawFixed, looking at the spec (step 2-3 seem to say that we're including all the integer digits of a floating point number, no matter how many that is, unless you configure your NumberFormat with singificant digits), but I wonder if we might want to treat this as something to change in the spec rather than to bring implementations in line with this version. By this reading, we'd also expect to see these less significant digits in a test case like, this, right:

"12344501000000000487815444678311936..toLocaleString()"

However, V8, SpiderMonkey and ChakraCore all evaluate to 12,344,501,000,000,000,000,000,000,000,000,000 whereas you might expect other digits at the end, as in the test expectations you include in this patch.

In V8's source, it looks like we're implementing the spec's logic for all the options and passing them straight through to ICU; I'd guess the other engines might do similarly, and the logic for rounding here comes from ICU (though I haven't dug deep enough in ICU to see what's going on exactly).

Intuitively for me, what the engines give is the better answer than what the spec seems to imply; what do you think? It could be a good idea to check in this test to draw attention to the issue, but I hope we can think this over before implementing in engines.

About the form of the patch itself: this is an unimportant, optional point, but I'd prefer if you kept the old test, but updated the expectation, and possibly introduced a new case after it for the non-zero less significant digits. This will make it more clear that the existing behavior has to change, and it keeps us more honest to not remove the previous test inputs, only change the output when it needs to change.

cc @jungshik

@littledan
Copy link
Member

Sad, but seems correct. Could you put a reference in the text to the ECMA 402 bug tc39/ecma402#128 , so that when JavaScript implements see this failure, they will be directed to the open issue discussing whether the semantics are appropriate?

@Yaffle
Copy link
Contributor Author

Yaffle commented Mar 2, 2017

@leobalter , @littledan , Thanks.

@leobalter leobalter added this to Done in Q1 Bocoup & Google Mar 2, 2017
@gnarf gnarf moved this from Done this week to Really Done in Q1 Bocoup & Google Mar 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants