Closed
Bug 894658
Opened 11 years ago
Closed 11 years ago
Implement ES6 Array.prototype.{keys, entries}
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: bbenvie, Assigned: sankha)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, feature, Whiteboard: [DocArea=JS])
Attachments
(1 file, 4 obsolete files)
9.66 KB,
patch
|
Details | Diff | Splinter Review |
For ES6, all builtin iterables should have the three iterator-creating methods (keys, values, entries). `Array#values` was added during bug 875433, but `Array#entries` and `Array#keys` remain unimplemented. See ES6 spec (July 2013 revision): * Array.prototype.entries (section 15.4.3.25) An iterator that produces a key/value pair for each index in the array. > [...['a', 'b', 'c'].entries()] // produces.... > [['0', 'a'], ['1', 'b'], ['2', 'c']] * Array.prototype.keys (section 15.4.3.26) An iterator that produces an array of keys for to the indices in array. > [...['a', 'b', 'c'].keys()] // produces.... > ['0', '1', '2'] Array#entries, along with bug 894637, are required for the following to work as desired: > new Map(['a', 'b', 'c'])
Comment 1•11 years ago
|
||
Given how much trouble Array#values is causing, we should probably implement these sooner rather than later. @sankha, you wouldn't by any chance be interested in working on this? What with having done all the other stuff in this area.
Flags: needinfo?(sankha93)
Assignee | ||
Comment 2•11 years ago
|
||
Thanks Till. I will look into this.
Assignee: general → sankha93
Status: NEW → ASSIGNED
Flags: needinfo?(sankha93)
Assignee | ||
Comment 3•11 years ago
|
||
This is a WIP patch. I will be writing the tests. I am having problem in the Array.prototype.entries. It fails to compile with what I have written so I have commented it out. Can you help me out with this? All the current jit-tests are passing with this.
Attachment #777235 -
Flags: feedback?(jorendorff)
Reporter | ||
Comment 4•11 years ago
|
||
Comment on attachment 777235 [details] [diff] [review] WIP Patch v1 Review of attachment 777235 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsiter.cpp @@ +961,5 @@ > + } > + break; > + > + case ElementIteratorObject::Keys: > + args.rval().setInt32(i); This should be a string. @@ +966,5 @@ > + break; > + > + case ElementIteratorObject::Entries: > + /*Value pair[2]; > + pair[0].setInt32(i); This should be a string.
Reporter | ||
Comment 5•11 years ago
|
||
Ignore my last comment, the last TC39 meeting has agreed to change those to be numbers.
Updated•11 years ago
|
Keywords: dev-doc-needed
Comment 6•11 years ago
|
||
Comment on attachment 777235 [details] [diff] [review] WIP Patch v1 Don't put JS_ArrayEntries/Keys/Values in the API. I don't think anybody will use it; let's just define those functions in jsarray.cpp. Otherwise this seems fine. Alternatively these could be self-hosted. I don't know which is better.
Attachment #777235 -
Flags: feedback?(jorendorff) → feedback+
Assignee | ||
Comment 7•11 years ago
|
||
Addressed the previous feedback and moved the keys() and entries() out of the public JSAPI. I have added tests as well. But will this patch have any issues with the patch in bug 919948?
Attachment #777235 -
Attachment is obsolete: true
Attachment #816599 -
Flags: review?(jorendorff)
Comment 8•11 years ago
|
||
This patch should be reimplemented on top of bug 919948, I think. Sorry for the churn :/
Comment 9•11 years ago
|
||
Comment on attachment 816599 [details] [diff] [review] patch v2 Review of attachment 816599 [details] [diff] [review]: ----------------------------------------------------------------- Andy is right, this does need to be reimplemented, but the patch should actually be much much easier now! In jsarray.cpp: > JS_SELF_HOSTED_FN("@@iterator", "ArrayValues", 0,0), All we need, I think, is to add some lines in this array for "keys", "values", and "entries". Plus tests, of course. Clearing the r? flag.
Attachment #816599 -
Flags: review?(jorendorff)
Assignee | ||
Comment 10•11 years ago
|
||
Should I add Array#values in this bug? It had caused a lot of trouble in bug 875433.
Flags: needinfo?(jorendorff)
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #816599 -
Attachment is obsolete: true
Attachment #822893 -
Flags: review?(jorendorff)
Comment 13•11 years ago
|
||
Comment on attachment 822893 [details] [diff] [review] patch v3 Review of attachment 822893 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. This needs more tests. Put the tests you've written into js/src/jit-test/tests/for-of, and check out the existing tests js/src/jit-test/tests/for-of/array-iterator-*.js. You can add test coverage just by modifying some of those existing test files to try .entries and .keys too. It's not necessary to modify them all, just a few here and there. Particularly array-iterator-generic.js.
Attachment #822893 -
Flags: review?(jorendorff)
Assignee | ||
Comment 14•11 years ago
|
||
Modified existing tests to include .keys() and .entries().
Attachment #822893 -
Attachment is obsolete: true
Attachment #825176 -
Flags: review?(jorendorff)
Comment 15•11 years ago
|
||
Comment on attachment 825176 [details] [diff] [review] patch v4 Review of attachment 825176 [details] [diff] [review]: ----------------------------------------------------------------- Great! Thanks again. r=me with these tweaks. ::: js/src/jit-test/tests/for-of/array-iterator-changing.js @@ +12,5 @@ > assertIteratorNext(it, 2000); > assertIteratorDone(it, undefined); > + > +// test that .keys() and .entries() have the same behaviour > + To really test the same behavior, this needs to set arr = [0, 1, 2]; at the beginning of the second test. ::: js/src/jit-test/tests/for-of/array-iterator-surfaces-2.js @@ +27,5 @@ > check(Array.prototype[std_iterator].call({})); > +check([].keys()); > +check(Array.prototype[std_iterator].call({})); > +check([].entries()); > +check(Array.prototype[std_iterator].call({})); I think you meant Array.prototype.keys.call({}) on line 29 and Array.prototype.entries.call({}) on line 31.
Attachment #825176 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 16•11 years ago
|
||
Made required changes.
Attachment #825176 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 17•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/31e1a183f9c1
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/31e1a183f9c1
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•10 years ago
|
Whiteboard: [DocArea=JS]
Comment 20•10 years ago
|
||
This feature breaks Twitter: https://bugzilla.mozilla.org/show_bug.cgi?id=924386#c19 We can (and will) evangelise that - I wonder if other stuff will break, though..
Comment 21•10 years ago
|
||
Release notes: https://developer.mozilla.org/en-US/Firefox/Releases/28#JavaScript New pages contributed by https://developer.mozilla.org/en-US/profiles/vladikoff: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/entries https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/keys We still need Array Iterator docs, but not tracking that here. As always: Feel free to review and improve these docs in the MDN wiki.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•