Closed
Bug 869996
Opened 11 years ago
Closed 11 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•11 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•11 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•11 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•11 years ago
|
||
Assignee: general → sankha93
Attachment #753127 -
Attachment is obsolete: true
Attachment #753764 -
Flags: feedback?(bbenvie)
Reporter | ||
Comment 5•11 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•11 years ago
|
Attachment #753764 -
Flags: review?(jorendorff)
Assignee | ||
Comment 6•11 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•11 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•11 years ago
|
||
Attachment #753764 -
Attachment is obsolete: true
Attachment #754046 -
Flags: review?(jorendorff)
Comment 9•11 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•11 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•11 years ago
|
||
Monday was a holiday. I'll land this today.
Comment 12•11 years ago
|
||
Finally got around to this, pushed to try server: https://tbpl.mozilla.org/?tree=Try&rev=7c2c8bfd4b33
Comment 13•11 years ago
|
||
Trying again: https://tbpl.mozilla.org/?tree=Try&rev=c38f68d528f1
Assignee | ||
Comment 14•11 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•11 years ago
|
||
I pushed to Try this time: https://tbpl.mozilla.org/?tree=Try&rev=6c13ecf09710
Comment 16•11 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•11 years ago
|
||
I spoke with Rick on IRC. This is of course an incremental step. We will implement the standard.
Comment 18•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/118b1725ac1e
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Updated•11 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 19•11 years ago
|
||
Filed bug 881226 to change the methods to the latest spec.
Comment 20•11 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
•