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

Change ToInteger(-0) to return +0? #1637

Closed
anba opened this issue Jul 19, 2019 · 9 comments · Fixed by #1827
Closed

Change ToInteger(-0) to return +0? #1637

anba opened this issue Jul 19, 2019 · 9 comments · Fixed by #1827

Comments

@anba
Copy link
Contributor

anba commented Jul 19, 2019

While working on some SpiderMonkey JIT compiler optimisations, I was wondering if it makes sense to change ToInteger(-0) to return +0 instead of -0, because in all but one case, the result of ToInteger(-0) is either explicitly or implicitly changed from -0 to +0 anyway.


Example for implicit conversion from String.prototype.startsWith:

  • An implicit conversion through the mathematical function max.
  1. If endPosition is undefined, let pos be len; else let pos be ? ToInteger(endPosition).
  2. Let end be min(max(pos, 0), len).

And from String.prototype.charAt:

  • An implicit conversion because -0 is treated the same as +0 for index positions.
  1. Let position be ? ToInteger(pos).
    ...
  2. Return the String value of length 1, containing one code unit from S, namely the code unit at index position.

And from Array.prototype.includes:

  • Implicit conversion, because ToString(-0) = ToString(+0) = "0" holds.
  1. Let n be ? ToInteger(fromIndex).
    ...
  2. If n ≥ 0, then
    a. Let k be n.
    ...
  3. Repeat, while k < len
    a. Let elementK be the result of ? Get(O, ! ToString(k)).

Example for explicit conversion from Array.prototype.indexOf:

  1. Let n be ? ToInteger(fromIndex).
    ...
  2. If n ≥ 0, then
    a. If n is -0, let k be +0; else let k be n.

Or TimeClip:

  1. Let clippedTime be ! ToInteger(time).
  2. If clippedTime is -0, set clippedTime to +0.

The only place where changing ToInteger(-0) to return +0 instead of -0 would make a difference, is Atomics.store:

  1. Let v be ? ToInteger(value).
    ...
  2. Return v.

So, changing ToInteger(-0) return +0 would allow to remove some explicit -0 to +0 conversions in various operations, but also result in a noticeable change for Atomics.store, (which we could avoid if necessary, but I'm not actually sure anyone would really notice the difference for that function when +0 instead of -0 would be returned.) Does it sound useful to anyone else to make this change?

@ljharb
Copy link
Member

ljharb commented Jul 19, 2019

Do 402 or HTML use our ToInteger at all?

@anba
Copy link
Contributor Author

anba commented Jul 19, 2019

I didn't find any occurrence of ToInteger at https://tc39.es/ecma402/ and https://html.spec.whatwg.org/.

@domenic
Copy link
Member

domenic commented Jul 19, 2019

Don't forget to check https://heycam.github.io/webidl/, but yeah, no ToInteger there either.

@anba
Copy link
Contributor Author

anba commented Jul 19, 2019

(And now the usual note: Hopefully we get one day some automatic tooling to help find downstream users resp. make it so that downstream users are notified on behavioural changes.

For example yesterday some Web App manifest test was failing because I changed the result of CanonicalizeLanguageTag from ECMA-402. :-/)

@ljharb
Copy link
Member

ljharb commented Jul 19, 2019

cc @syg for Atomics.store question

@ljharb ljharb added the editor call to be discussed in the next editor call label Jan 3, 2020
@syg
Copy link
Contributor

syg commented Jan 3, 2020

I highly doubt that changing the semantics of Atomics.store to always truncate -0 would be a web compat issue, especially since SharedArrayBuffers are just getting turned back on again across browsers.

Editorially it seems helpful to set the precedent for future spec writers to not have to think about whether they would want to truncate -0 or not when returning integral numbers back to script. Implementation-wise both the truncation and no truncation variants of ToInteger would still be implemented, since there are call sites where the result Number can never be -0 or the sign bit pattern doesn't matter so there's no reason to emit the extra branch to truncate. And I suppose the current no-truncation behavior for Atomics.store saves a branch for Atomics.store as well.

All in all I think I am in favor of trying this change -- I like the mental model of methods that return some kind of integral coercion as always doing truncation. -0 isn't an integer.

@ljharb ljharb removed the editor call to be discussed in the next editor call label Jan 3, 2020
@ljharb
Copy link
Member

ljharb commented Jan 3, 2020

I'm also in favor of it. I'll put up a PR.

@ljharb

This comment has been minimized.

@ljharb

This comment has been minimized.

ljharb added a commit to ljharb/ecma262 that referenced this issue Jan 3, 2020
Fixes tc39#1637.

This only has an observable impact on `Atomics.store`.
ljharb added a commit to ljharb/ecma262 that referenced this issue Jan 3, 2020
Fixes tc39#1637.

This only has an observable impact on `Atomics.store`.
ljharb added a commit to ljharb/ecma262 that referenced this issue Jan 6, 2020
Fixes tc39#1637.

This only has an observable impact on `Atomics.store`.
ljharb added a commit to ljharb/ecma262 that referenced this issue Jan 8, 2020
Fixes tc39#1637.

This only has an observable impact on `Atomics.store`.
ljharb added a commit to ljharb/ecma262 that referenced this issue Feb 1, 2020
Fixes tc39#1637.

This only has an observable impact on `Atomics.store`.
@ljharb ljharb closed this as completed in ddac91d Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants