Closed Bug 866849 Opened 12 years ago Closed 11 years ago

Implement ES6 Array.of

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: evilpies, Assigned: jorendorff)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, Whiteboard: [mentor=evilpies])

Attachments

(4 files, 2 obsolete files)

No description provided.
Summary: Implement Array.from and Array.of → Implement ES6 Array.from and Array.of
We should implement this via self-hosting. You would want to look at array.js. Take a look at bug 715181, if you are interested in how this would look like.
Whiteboard: [mentor=evilpies]
I would like to take this up.
You got it :)
Assignee: general → sankha93
Sankha are you looking into this?
Flags: needinfo?(sankha93)
Attached patch WIP Patch (obsolete) — Splinter Review
I just wrote the base function. Can you just see if they are alright? I'll be writing the tests next.
Attachment #753958 - Flags: feedback?(evilpies)
Flags: needinfo?(sankha93)
Attached patch WIP PatchSplinter Review
Please ignore the previous one. I had forgotten to update jsarray.cpp.
Attachment #753958 - Attachment is obsolete: true
Attachment #753958 - Flags: feedback?(evilpies)
Attachment #753963 - Flags: feedback?(evilpies)
Comment on attachment 753963 [details] [diff] [review] WIP Patch Review of attachment 753963 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/Array.js @@ +149,5 @@ > + > + // Steps 4-5 > + var A; > + if (isConstructor(C)) { > + var newObj = C(len); Should be `new C(len)` @@ +221,5 @@ > + var putStatus = Put(A, "length", len, true); > + return A; > +} > + > +function ArrayStaticOf(list, arrayLike, mapfn = undefined, thisArg = undefined) { ArrayStaticFrom
That doesn't really work right? Because of DefinePropertyOrThrow and Put. Now that I think about, I probably told you some of the things I found on IRC already. You should maybe do this after you set up all the infrastructure in Bug 866847.
Attached patch v1Splinter Review
Stealing this bug. If we can simplify the implementation by throwing this C++ code away and writing it in JS, *great*! But let's try and ship something. This first patch may appear to have nothing to do with anything. It defines an assertEqual function that can be used in the test suite; it's sort of like the arraysEqual function in array-compare.js, except that 1. you can use it on any object, not just arrays; 2. assertEqual is to assert∘arraysEqual as deep copy is to shallow copy: assertEqual([{}], [{}]) passes because {} "equals" {}, whereas arraysEqual([{}], [{}]) returns false because {} !== {}; 3. it's otherwise a lot more thorough, checking property attributes, extensibility, getters and setters, etc.; and 4. it generates nice error messages. (has to because of deep equivalence) Etymology: Scheme uses a predicate named eq? to test identity equivalence and one named equal? to test structural equivalence. That's where assertEq's name comes from. I'll use this function in tests later on.
Assignee: sankha93 → jorendorff
Attachment #766382 - Flags: review?(evilpies)
Attached patch Part 2 - Array.of, v1 (obsolete) — Splinter Review
Attachment #766383 - Flags: review?(evilpies)
Same as v1, but a few more tests.
Attachment #766383 - Attachment is obsolete: true
Attachment #766383 - Flags: review?(evilpies)
Attachment #766389 - Flags: review?(evilpies)
This isn't quite finished. I need to change the type of the loop variable from uint32_t to double. Good night!
Two more changes to Array.from appear to have consensus, according to the TC39 meeting notes, but are not yet in the spec: - Array.from will accept an iterable argument, not just an arraylike. It will check whether the argument is iterable first, and fall back on arraylike. https://github.com/rwldrn/tc39-notes/blob/master/es6/2012-11/nov-29.md#revisit-nov-27-resolution-on-iterables-in-spread - Array.from will accept two extra optional arguments: Array.from(iterableOrArraylike, mapFn, thisArg) https://github.com/rwldrn/tc39-notes/blob/master/es6/2013-01/jan-30.md#revising-the-array-subclassing-kind-issue I'm going to ask a few questions about these changes on es-discuss.
Attachment #753963 - Flags: feedback?(evilpies)
Comment on attachment 766382 [details] [diff] [review] v1 Review of attachment 766382 [details] [diff] [review]: ----------------------------------------------------------------- This is quite neat! I however still feel that semantics are somewhat surprising. Maybe it's not the best name for this function. ::: js/src/jit-test/lib/asserts.js @@ +106,5 @@ > + newError = Error, > + has = call.bind(Map.prototype.has), > + get = call.bind(Map.prototype.get), > + set = call.bind(Map.prototype.set), > + toString = call.bind(Object.prototype.toString), I would name all functions like Map_has, Object_toString. I think that fits with that we have been doing for self-hosting. @@ +117,5 @@ > + join = call.bind(Array.prototype.join), > + _uneval = uneval; > + > + let isPrimitive = function isPrimitive(v) { > + return v === null || I thought about switching this to be switch(typeof), but that makes the object and undefined case very confusing. We always have to make a strict compare with undefined, because of document.all. You should add that as a comment here. @@ +133,5 @@ > + throw new Error(exc.message + (msg ? " " + msg : "")); > + } > + }; > + > + let assertSameClass = function assertSameClass(a, b, msg) { why sometimes arrow and sometimes not?
Attachment #766382 - Flags: review?(evilpies) → review+
Comment on attachment 766389 [details] [diff] [review] Part 2 - Array.of, v2 Review of attachment 766389 [details] [diff] [review]: ----------------------------------------------------------------- The functions really makes these tests look reasonable. ::: js/src/jsarray.cpp @@ +2686,5 @@ > } > > +// ES6 9.2.5 IsConstructor > +static bool > +IsConstructor(const Value &v) This should probably be in jsfun.h
Attachment #766389 - Flags: review?(evilpies) → review+
(In reply to Tom Schuster [:evilpie] from comment #14) > This is quite neat! I however still feel that semantics are somewhat > surprising. Maybe it's not the best name for this function. You didn't like the Scheme analogy? :-) I admit it's not exactly self-explanatory. I'll ask around for a new name, but I'll let that naming decision race freely with landing this. > I would name all functions like Map_has, Object_toString. I think that fits > with that we have been doing for self-hosting. Done. > I thought about switching this to be switch(typeof), but that makes the > object and undefined case very confusing. We always have to make a strict > compare with undefined, because of document.all. You should add that as a > comment here. Yow! I had forgotten about typeof document.all. Commented. > why sometimes arrow and sometimes not? Uhrrr.... because I wrote this without proper caffienation? Switched to arrows throughout, except assertEqual itself which remains a plain function, to match the others in this file.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
I went with assertDeepEq, btw.
I see this was resolved fixed but it looks like Array.from was never reviewed and landed. Also in the meantime the new ES6 spec came out that does include Array.from with the signature `Array.from(arrayLike, mapfn=undefined, thisArg=undefined)`.
You are right, the Array.from patches never landed and we need to update them to the latest spec. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 903755
In a new bug please?
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
(In reply to Jason Orendorff [:jorendorff] from comment #21) > In a new bug please? bug 904723
Summary: Implement ES6 Array.from and Array.of → Implement ES6 Array.of
Firefox 25 is out now and this still lacks some documentation. I expect this to be described here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/of Sebastian
Sebastian, it is a wiki, just do it. There is no guarantee when a doc will be written, we have not enough resources to do it all.
(In reply to Jean-Yves Perrier [:teoli] from comment #24) > Sebastian, it is a wiki, just do it. There is no guarantee when a doc will > be written, we have not enough resources to do it all. I don't mind doing the docs. I wrote the spec proposal so I have a pretty good understanding :)
> (In reply to Jean-Yves Perrier [:teoli] from comment #24) > Sebastian, it is a wiki, just do it. Well, I didn't know what it does. So I wanted to have a look at the wiki page... > we have not enough resources to do it all. I know this problem very well. Sebastian(In reply to Rick Waldron [:rwaldron] from comment #25) > I don't mind doing the docs. I wrote the spec proposal so I have a pretty > good understanding :) That would be fantastic. Sebastian
(In reply to Sebastian Zartner from comment #26) > > (In reply to Jean-Yves Perrier [:teoli] from comment #24) > > Sebastian, it is a wiki, just do it. > > Well, I didn't know what it does. So I wanted to have a look at the wiki > page... > > > we have not enough resources to do it all. > > I know this problem very well. > > Sebastian(In reply to Rick Waldron [:rwaldron] from comment #25) > > I don't mind doing the docs. I wrote the spec proposal so I have a pretty > > good understanding :) > > That would be fantastic. > > Sebastian Done! https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/of
Perfect! Thanks a lot. Sebastian
Florian reviewed the page. Thanks all and especially rick who wrote it!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: