Disallow -0 in algorithm for checking array index #517
Conversation
index.bs
Outdated
| 1. If |P| is "<code>-0</code>", then return <emu-val>false</emu-val>. | ||
|
|
||
| Note: <a abstract-op>CanonicalNumericIndexString</a> allows "<code>-0</code>" as a special | ||
| case, even though it is not an [=integer index=] or by extension an [=array index=]. |
There was a problem hiding this comment.
If this indeed matches implementations, wouldn't it be cleaner to return false if index is -0 after the following step?
There was a problem hiding this comment.
The implementation for this in Chromium:
https://chromium.googlesource.com/v8/v8.git/+/842d424d99060375bf4a8b6fdfd2a0177e4c0477/src/api.cc#4128
https://chromium.googlesource.com/v8/v8.git/+/842d424d99060375bf4a8b6fdfd2a0177e4c0477/src/utils-inl.h#35
So no special handling of "-0" there because the code uses unsigned integers. ES uses ToNumber where negative zero is possible, so whatever this says won't be a close match.
|
I have tests for the implications fo |
|
FWIW, I wonder how many other algorithms have subtle -0 issues... |
There was a problem hiding this comment.
This special-casing LGTM. Checking for -0 would also work, but something more like https://tc39.github.io/ecma262/#array-index would also be fine as long as the note makes it clear that it's "-0" that is special.
|
Wouldn't just changing step 4 from: to: have the same effect, and match the ES spec more closely? Maybe we could also mention -0 in the note right below, or just say that the range corresponds to the range of supported array indices in ECMAScript. |
|
With a note saying that it's supposed to be equivalent to https://tc39.github.io/ecma262/#array-index anything would do, but using just the ≤ operator in the normative definition seems wrong. Whether operating on IEEE doubles or real numbers, positive zero equals negative zero. So it's not possible to write that expression out as-is in code or in mathematics and have -0 not be in the range. |
|
Hopefully this will all be a bit clearer once whatwg/infra#87 is solved. |
Yeah, I tend to agree, but as I lifted that off of the ES spec I get to blame someone else. :D |
|
Who can make a decision on this? Anything WFM. |
|
A bunch of suggestions were made to improve the initial PR. I'll make a last one: replace step 5 with the following: and fix or remove the related note. |
|
Realized how tautological my previous comment was. Pushed some changes to @TimothyGu's branch. Fixed the issue that we weren't filtering out non integers before. @foolip can you give the latest a look? |
index.bs
Outdated
| 1. If |index| is -0, then return <emu-val>false</emu-val>. | ||
| 1. If |index| is smaller than 0, then return <emu-val>false</emu-val>. | ||
| 1. If |index| is greater than or equal to 2<sup>32</sup> − 1, | ||
| then return <emu-val>false</emu-val>. |
There was a problem hiding this comment.
I would use < and ≥, just to make things a bit more obvious. If you prefer English over mathematical symbols, s/smaller than/less than/.
Also I'd like to keep the note:
Note: 232 − 1 is the maximum array length allowed by ECMAScript.
There was a problem hiding this comment.
I was hesitant about removing the note, but if it's not just me, then I'll keep it around.
|
Yay, thanks. Writing the test and merging this. |
Fixes: #516
CC @foolip
Preview | Diff