Closed Bug 894658 Opened 12 years ago Closed 12 years ago

Implement ES6 Array.prototype.{keys, entries}

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

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)

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'])
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)
Thanks Till. I will look into this.
Assignee: general → sankha93
Status: NEW → ASSIGNED
Flags: needinfo?(sankha93)
Attached patch WIP Patch v1 (obsolete) — Splinter Review
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)
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.
Ignore my last comment, the last TC39 meeting has agreed to change those to be numbers.
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+
Attached patch patch v2 (obsolete) — Splinter Review
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)
This patch should be reimplemented on top of bug 919948, I think. Sorry for the churn :/
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)
Should I add Array#values in this bug? It had caused a lot of trouble in bug 875433.
Flags: needinfo?(jorendorff)
Er, no, definitely not. Good catch.
Flags: needinfo?(jorendorff)
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #816599 - Attachment is obsolete: true
Attachment #822893 - Flags: review?(jorendorff)
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)
Attached patch patch v4 (obsolete) — Splinter Review
Modified existing tests to include .keys() and .entries().
Attachment #822893 - Attachment is obsolete: true
Attachment #825176 - Flags: review?(jorendorff)
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+
Attached patch patch v5Splinter Review
Made required changes.
Attachment #825176 - Attachment is obsolete: true
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [DocArea=JS]
JS feature added in Firefox 28
Keywords: feature
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..
Depends on: 987762
Depends on: 924386
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: