Closed
Bug 885553
Opened 10 years ago
Closed 10 years ago
Implement ES6 Array.prototype.find and Array.prototype.findIndex
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: bbenvie, Assigned: till)
References
(Blocks 1 open bug)
Details
(Keywords: doc-bug-filed)
Attachments
(1 file, 2 obsolete files)
7.69 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
These are basically the same as `Array.prototype.some` (returns on first truthy result from callback) except they return different things: * Array.prototype.find(predicate, thisArg = undefined) Returns the value or undefined if not found. * Array.prototype.findIndex(predicate, thisArg = undefined) Returns the index or -1 if not found. See ES6 spec (May 2013 revision) sections 15.4.3.24 and 15.4.3.25. Also see: https://github.com/rwldrn/tc39-notes/blob/master/es6/2013-03/mar-14.md#410-array-extras
Reporter | ||
Comment 1•10 years ago
|
||
Sorry, that's ES6 spec sections 15.4.3.23 and 15.4.3.24.
Assignee | ||
Updated•10 years ago
|
Assignee: general → tschneidereit
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
Implements both methods, but lacking tests
Comment 3•10 years ago
|
||
Step 9.d.i of Array.prototype.find is not properly implemented due to possible side-effects in `Get(O, pk)`. For example `Array.prototype.find.call({get 0(){ print("get zero") }, length: 1}, ()=>true)` should print "get zero" only once.
Assignee | ||
Comment 4•10 years ago
|
||
Now with tests. I heavily cribbed from the Array.prototype.some tests in js1_6/Array/regress-290592.js, but added tests for array-likes and removed the method-as-callback tests, as I didn't see how they could be interesting.
Attachment #765687 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•10 years ago
|
Attachment #765647 -
Attachment is obsolete: true
Assignee | ||
Comment 5•10 years ago
|
||
@André, good catch! Updated with a fix and test for that case.
Attachment #765712 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•10 years ago
|
Attachment #765687 -
Attachment is obsolete: true
Attachment #765687 -
Flags: review?(jwalden+bmo)
Comment 6•10 years ago
|
||
Comment on attachment 765712 [details] [diff] [review] Implement ES6 Array.prototype.find and Array.prototype.findIndex Review of attachment 765712 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/Array.js @@ +397,5 @@ > + /* Steps 1-2. */ > + var O = ToObject(this); > + > + /* Steps 3-5. */ > + var len = TO_UINT32(O.length); Per spec this should be ToInteger. Please add a test that distinguishes these, something like should do it: var obj = { length: -4294967295, 0: 42 }; assertEq(Array.prototype.find.apply(obj, () => true), undefined); @@ +410,5 @@ > + var T = arguments.length > 1 ? arguments[1] : undefined; > + > + /* Steps 8-9. */ > + /* Steps a (implicit), and e. */ > + for (var k = 0; k < len; k++) { The spec algorithm is in terms of mathematically precise numbers. This is in terms of IEEE-754 numbers. So if someone does something like this: var obj = { 18014398509481984: true, length: 18014398509481988 }; Array.prototype.find.apply(obj, () => true); our implementation will hang forever. Let's at least put a comment by this pointing out the issue, even if we're not going to deal with it now. @@ +430,5 @@ > + /* Steps 1-2. */ > + var O = ToObject(this); > + > + /* Steps 3-5. */ > + var len = TO_UINT32(O.length); ToInteger, and same variety of test please. ::: js/src/tests/ecma_6/Array/find_findindex.js @@ +31,5 @@ > +var obj; > +var strings = ['hello', 'Array', 'WORLD']; > +var mixed = [0, '1', 2]; > +var sparsestrings = new Array(); > +sparsestrings[2] = 'sparse'; Please also test an array that isIndexed() -- Object.defineProperty(arr, someIndex, { get: function() { return 42; } }) should do the trick of setting one up. @@ +37,5 @@ > + > +// find and findIndex have 1 required argument > + > +expect = 1; > +actual = Array.prototype.find.length; var-declare expect/actual, please, somewhere. @@ +108,5 @@ > +catch(e) > +{ > + actual = dumpError(e); > +} > +reportCompare(expect, actual, 'mixed: findIndex finds first string element'); reportCompare compares by loose equality, so checking for 1 and '1' is going to admit more cases than you want. I don't really care what you use, so long as you don't admit extra cases, but I'd have used assertEq for this stuff, because it uses a SameValue check to determine equality -- and that has a same-type requirement in it (and same-signed-zero and NaNs-are-equal bits, but those aren't relevant here). Anyway, use *something* that won't let both cases through, please.
Attachment #765712 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a839287871f9 and, for tests I forgot to qref: https://hg.mozilla.org/integration/mozilla-inbound/rev/dc67ab8bef36
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a839287871f9 https://hg.mozilla.org/mozilla-central/rev/dc67ab8bef36
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Updated•10 years ago
|
Keywords: dev-doc-needed
Comment 9•10 years ago
|
||
Follow up for documentation on https://bugzilla.mozilla.org/show_bug.cgi?id=891779
Keywords: dev-doc-needed → doc-bug-filed
Assignee | ||
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•