Closed
Bug 894658
Opened 12 years ago
Closed 12 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•12 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•12 years ago
|
||
Thanks Till. I will look into this.
Assignee: general → sankha93
Status: NEW → ASSIGNED
Flags: needinfo?(sankha93)
Assignee | ||
Comment 3•12 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•12 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•12 years ago
|
||
Ignore my last comment, the last TC39 meeting has agreed to change those to be numbers.
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 6•12 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•12 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•12 years ago
|
||
This patch should be reimplemented on top of bug 919948, I think. Sorry for the churn :/
Comment 9•12 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•12 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•12 years ago
|
||
Attachment #816599 -
Attachment is obsolete: true
Attachment #822893 -
Flags: review?(jorendorff)
Comment 13•12 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•12 years ago
|
||
Modified existing tests to include .keys() and .entries().
Attachment #822893 -
Attachment is obsolete: true
Attachment #825176 -
Flags: review?(jorendorff)
Comment 15•12 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•12 years ago
|
||
Made required changes.
Attachment #825176 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 17•12 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•11 years ago
|
Whiteboard: [DocArea=JS]
Comment 20•11 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•11 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
•