Closed Bug 894658 Opened 11 years ago Closed 11 years ago

Implement ES6 Array.prototype.{keys, entries}


(Core :: JavaScript Engine, defect)

Not set





(Reporter: bbenvie, Assigned: sankha)


(Blocks 1 open bug)


(Keywords: dev-doc-complete, feature, Whiteboard: [DocArea=JS])


(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
  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
  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
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{}) on line 29
and{}) 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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 958095
Whiteboard: [DocArea=JS]
JS feature added in Firefox 28
Keywords: feature
This feature breaks Twitter:
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.