Closed
Bug 869996
Opened 12 years ago
Closed 12 years ago
Set.prototype.{keys, values, entries}
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: bbenvie, Assigned: sankha)
References
(Blocks 1 open bug)
Details
(Keywords: doc-bug-filed)
Attachments
(1 file, 4 obsolete files)
13.60 KB,
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 years ago
|
||
Assignee: general → sankha93
Attachment #753127 -
Attachment is obsolete: true
Attachment #753764 -
Flags: feedback?(bbenvie)
Reporter | ||
Comment 5•12 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+
Updated•12 years ago
|
Attachment #753764 -
Flags: review?(jorendorff)
Assignee | ||
Comment 6•12 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 7•12 years ago
|
||
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•12 years ago
|
||
Attachment #753764 -
Attachment is obsolete: true
Attachment #754046 -
Flags: review?(jorendorff)
Comment 9•12 years ago
|
||
(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 10•12 years ago
|
||
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+
Comment 11•12 years ago
|
||
Monday was a holiday. I'll land this today.
Comment 12•12 years ago
|
||
Finally got around to this, pushed to try server:
https://tbpl.mozilla.org/?tree=Try&rev=7c2c8bfd4b33
Comment 13•12 years ago
|
||
Trying again:
https://tbpl.mozilla.org/?tree=Try&rev=c38f68d528f1
Assignee | ||
Comment 14•12 years ago
|
||
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
Assignee | ||
Comment 15•12 years ago
|
||
I pushed to Try this time: https://tbpl.mozilla.org/?tree=Try&rev=6c13ecf09710
Comment 16•12 years ago
|
||
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
Comment 17•12 years ago
|
||
I spoke with Rick on IRC. This is of course an incremental step. We will implement the standard.
Comment 18•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Updated•12 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 19•12 years ago
|
||
Filed bug 881226 to change the methods to the latest spec.
Comment 20•12 years ago
|
||
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.
Description
•