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

Normative: explicitly specify order of operations in MakeTime #2120

Merged
merged 1 commit into from
Oct 7, 2020

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Jul 31, 2020

While reviewing #2007, @michaelficarra and I noticed that MakeTime is underspecified: it has a step which reads

Let t be h * msPerHour + m * msPerMinute + s * msPerSecond + milli, performing the arithmetic according to IEEE 754-2019 rules (that is, as if using the ECMAScript operators * and +)

but does not specify in which order to perform the additions. The choice of order is observable when using IEEE double-precision floats: for example,

  • ((80063993375 * 3600000 + 29 * 60000) + 1 * 1000) + -288230376151711744 is 29312, but
  • 80063993375 * 3600000 + (29 * 60000 + (1 * 1000 + -288230376151711744)) is 29248.

(The actual value is 29256.) So Date.UTC(1970, 0, 1, 80063993375, 29, 1, -288230376151711740) can have either of those results depending on your choice of order.

SpiderMonkey, ChakraCore, V8, and XS all appear to perform the arithmetic in left-to-right order, producing 29312. Graal produces 29248. (JSC bounds all arguments above by 2^31, so it cannot observe this case, and quickJS produces 29256 because it internally uses 64-bit integers for this arithmetic, rather than the IEEE floats required, and so does not lose precision.)

In this PR I've chosen to go with the majority and enforce left-to-right evaluation order.

@jmdyck
Copy link
Collaborator

jmdyck commented Jul 31, 2020

The phrase "as if using the ECMAScript operators * and +" suggests to also use ECMAScript associativity, which agrees with this PR.

@michaelficarra michaelficarra added needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 spec bug labels Jul 31, 2020
@ljharb ljharb requested review from michaelficarra, syg and a team August 1, 2020 04:58
@ljharb ljharb self-assigned this Sep 11, 2020
@ljharb ljharb added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Sep 21, 2020
@bakkot
Copy link
Contributor Author

bakkot commented Sep 28, 2020

Tests: tc39/test262#2831

@rwaldron rwaldron added has test262 tests and removed needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants