Set.prototype.{keys, values, entries}

RESOLVED FIXED in mozilla24

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bbenvie, Assigned: sankha)

Tracking

(Blocks: 1 bug, {doc-bug-filed})

unspecified
mozilla24
doc-bug-filed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
The ES6 spec specifies [1] that `Set.prototype` should have the following methods:

* entries - returns a SetIterator where each Set entry is returned in an array twice, `[value, value]`
* keys - returns a SetIterator that iterates through the values in the Set
* values - same as `keys`

See bug 817368 for Map.prototype.{keys,values,entries}.

[1] http://people.mozilla.org/~jorendorff/es6-draft.html#sec-15.16.4.5
(Reporter)

Updated

6 years ago
Blocks: 694100
(Assignee)

Comment 1

6 years ago
Posted patch patch v1 (obsolete) — Splinter Review
I have written a WIP patch looking at the implementation of Map.prototype.{keys, values, entries}. There are no test cases yet. Can you tell me if I am going in the right direction?
Attachment #753119 - Flags: feedback?(bbenvie)
(Assignee)

Comment 2

6 years ago
Posted patch patch v1 (obsolete) — Splinter Review
Oops! That was a wrong patch. Can you provide feedback for this one?
Attachment #753119 - Attachment is obsolete: true
Attachment #753119 - Flags: feedback?(bbenvie)
Attachment #753127 - Flags: feedback?(bbenvie)
(Reporter)

Comment 3

6 years ago
Comment on attachment 753127 [details] [diff] [review]
patch v1

Review of attachment 753127 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good overall, with a few tweaks.

::: js/src/builtin/MapObject.cpp
@@ +1465,5 @@
>      return static_cast<ValueSet::Range *>(getSlot(RangeSlot).toPrivate());
>  }
>  
> +inline SetObject::IteratorKind
> +MapIteratorObject::kind() const

Should be `SetIteratorObject`

@@ +1537,5 @@
>          thisobj.setReservedSlot(RangeSlot, PrivateValue(NULL));
>          return js_ThrowStopIteration(cx);
>      }
>  
> +    SetObject::IteratorKind kind = thisobj.kind();

`MapIteratorObject::next_impl` uses a switch here. It might be good to do the same here since you can just fall through for Keys/Values, due to them being the same.

@@ +1608,1 @@
>      JS_FN("iterator", iterator, 0, 0),

The ES6 spec says that `Set.prototype.@@iterator` is the same as `Set.prototype.values`. You can remove `SetObject::iterator` entirely and just set both "values" and "iterator" to `SetObject::values`. (Map does this with `entries`, for comparison)
Attachment #753127 - Flags: feedback?(bbenvie) → feedback-
(Assignee)

Comment 4

6 years ago
Posted patch patch v2 (obsolete) — Splinter Review
Assignee: general → sankha93
Attachment #753127 - Attachment is obsolete: true
Attachment #753764 - Flags: feedback?(bbenvie)
(Reporter)

Comment 5

6 years ago
Comment on attachment 753764 [details] [diff] [review]
patch v2

Review of attachment 753764 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm
Attachment #753764 - Flags: feedback?(bbenvie) → feedback+
Attachment #753764 - Flags: review?(jorendorff)
(Assignee)

Comment 6

6 years ago
I have just been able to run the jit-tests on my machine, and there is a problem with the Set-values-1.js file. I fear its failing the last case with the FAIL error. Can you please tell me what have I done wrong?
Comment on attachment 753764 [details] [diff] [review]
patch v2

Review of attachment 753764 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing the review flag. This is very much on the right track! Please refresh it and ask for review again.

::: js/src/builtin/MapObject.cpp
@@ +1540,5 @@
>  
> +    switch (thisobj.kind()) {
> +        case SetObject::Keys:
> +        case SetObject::Values:
> +            args.rval().set(range->front().get());

Tragically, the SpiderMonkey style guide requires 'case' labels to be indented just 2 spaces -- like in MapIteratorObject::next_impl.

@@ +1607,5 @@
>      JS_FN("delete", delete_, 1, 0),
> +    JS_FN("keys", keys, 0, 0),
> +    JS_FN("values", values, 0, 0),
> +    JS_FN("entries", entries, 0, 0),
> +    JS_FN("iterator", values, 0, 0),

The specification says that "keys", "values", and @@iterator are all the same function object.  Please add a test:
  assertEq(Set.prototype.keys, Set.prototype.values);
and another one for Map:
  assertEq(Map.prototype.iterator, Map.prototype.entries);

To make these tests pass, you will have to remove "keys", "values", and "iterator" from SetObject::methods. Then change SetObject::initClass to define those functions by hand. Unfortunately this is kind of a lot of code.

It'll be something like this:

> JSObject *
> SetObject::initClass(JSContext *cx, JSObject *obj)
> {
>     Rooted<GlobalObject*> global(cx, &obj->asGlobal());
>-    return InitClass(cx, global, &class_, JSProto_Set, construct, properties, methods);
>+    RootedObject proto(cx,
>+        InitClass(cx, global, &class_, JSProto_Set, construct, properties, methods));
>+    if (proto) {
>+        // Define the "values" method.
>+        JSFunction *fun = JS_DefineFunction(cx, proto, "values", values, 0, 0);
>+        if (!fun)
>+            return NULL;
>+
>+        // Define its aliases.
>+        RootedValue funval(cx, ObjectValue(*fun));
>+        if (!JS_DefineProperty(cx, proto, "keys", funval, NULL, NULL, 0))
>+            return NULL;
>+        if (!JS_DefineProperty(... "iterator" ...))
>+            return NULL;
>+    }
>+    return proto;
> }

Please do the same for "entries"/"iterator" in MapObject!

@@ +1823,1 @@
>  }

Please delete SetObject::keys.

::: js/src/builtin/MapObject.h
@@ +126,5 @@
>  };
>  
>  class SetObject : public JSObject {
>    public:
> +    enum IteratorKind { Keys, Values, Entries };

Only Values and Entries are necessary.

@@ +155,5 @@
> +    static JSBool keys(JSContext *cx, unsigned argc, Value *vp);
> +    static bool values_impl(JSContext *cx, CallArgs args);
> +    static JSBool values(JSContext *cx, unsigned argc, Value *vp);
> +    static bool entries_impl(JSContext *cx, CallArgs args);
> +    static JSBool entries(JSContext *cx, unsigned argc, Value *vp);

Please remove the declarations for keys and keys_impl.

::: js/src/jit-test/tests/collections/Set-values-2.js
@@ +13,5 @@
> +assertEq(ki.next(), 3);
> +assertEq(ki.next(), 4);
> +assertThrowsValue(function () { ki.next(); }, StopIteration);
> +
> +assertEq([k for (k of s.keys())].toSource(), [1, 2, 3, 4].toSource());

[k or (k of s.keys())] can be expressed like this:   [...s.keys()]
Attachment #753764 - Flags: review?(jorendorff)
(Assignee)

Comment 8

6 years ago
Posted patch final patch (obsolete) — Splinter Review
Attachment #753764 - Attachment is obsolete: true
Attachment #754046 - Flags: review?(jorendorff)
(In reply to Sankha Narayan Guria [:sankha93] from comment #6)
> I have just been able to run the jit-tests on my machine, and there is a
> problem with the Set-values-1.js file. I fear its failing the last case with
> the FAIL error. Can you please tell me what have I done wrong?

You can get the command lines and output from the tests by passing the options -so to the test runner. Like this:

    python jit-test/jit_test.py -so $YOUR_BUILD_DIR/js Set-values-1

Looks like it's a typo in the test, not a bug in your C++.
Comment on attachment 754046 [details] [diff] [review]
final patch

Review of attachment 754046 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. I'm going to rename some test files but other than that we're all set. I will land this on Monday! Thanks!
Attachment #754046 - Flags: review?(jorendorff) → review+
(Assignee)

Updated

6 years ago
Blocks: 875433
Monday was a holiday. I'll land this today.
Finally got around to this, pushed to try server:
  https://tbpl.mozilla.org/?tree=Try&rev=7c2c8bfd4b33
(Assignee)

Comment 14

6 years ago
Posted patch final patchSplinter Review
Just realized that the previous patch did not have my username and email set, so uploading the same patch with all the details.
Attachment #754046 - Attachment is obsolete: true
Comment on attachment 756136 [details] [diff] [review]
final patch

Review of attachment 756136 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/collections/Set-values-2.js
@@ +10,5 @@
> +var ki = s.keys();
> +assertEq(ki.next(), 1);
> +assertEq(ki.next(), 2);
> +assertEq(ki.next(), 3);
> +assertEq(ki.next(), 4);

Please see 15.16.6.2.2 SetIterator.prototype.next() of the latest specification draft.

.next() should produce an ItrResult object that has both a "value" and "done" property. The "value" property's own value will be as those shown above, the "done" property will be a boolean value that is |true| until there are no more values to yield, at which time it will have the boolean value |false|

@@ +11,5 @@
> +assertEq(ki.next(), 1);
> +assertEq(ki.next(), 2);
> +assertEq(ki.next(), 3);
> +assertEq(ki.next(), 4);
> +assertThrowsValue(function () { ki.next(); }, StopIteration);

See line 14 comments, StopIteration has been removed from the design of the feature
I spoke with Rick on IRC. This is of course an incremental step. We will implement the standard.
https://hg.mozilla.org/mozilla-central/rev/118b1725ac1e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Keywords: dev-doc-needed
(Assignee)

Comment 19

6 years ago
Filed bug 881226 to change the methods to the latest spec.
Depends on: 885260
Follow up for documentation: https://bugzilla.mozilla.org/show_bug.cgi?id=885260
Keywords: dev-doc-needed → doc-bug-filed
You need to log in before you can comment on or make changes to this bug.