Visitar URL original
Normative: explicitly specify order of operations in MakeTime by bakkot · Pull Request #2120 · tc39/ecma262 · GitHub
Skip to content

Normative: explicitly specify order of operations in MakeTime - #2120

Merged
ljharb merged 1 commit into
masterfrom
maketime-ooo
Oct 7, 2020
Merged

ljharb merged 1 commit into
masterfrom
maketime-ooo

Conversation

@bakkot

@bakkot bakkot commented Jul 31, 2020 •

Copy link
Copy Markdown
Member

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

jmdyck commented Jul 31, 2020

Copy link
Copy Markdown
Collaborator

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 a team, michaelficarra and syg 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

bakkot commented Sep 28, 2020

Copy link
Copy Markdown
Member Author

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
@ljharb
ljharb merged commit 2cf5a6f into master Oct 7, 2020
@ljharb
ljharb deleted the maketime-ooo branch October 7, 2020 23:00
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