Closed
Bug 866849
Opened 12 years ago
Closed 11 years ago
Implement ES6 Array.of
Categories
(Core :: JavaScript Engine, defect)
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)
5.20 KB,
patch
|
Details | Diff | Splinter Review | |
8.29 KB,
patch
|
evilpies
:
review+
|
Details | Diff | Splinter Review |
12.58 KB,
patch
|
evilpies
:
review+
|
Details | Diff | Splinter Review |
9.99 KB,
patch
|
Details | Diff | Splinter Review |
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]
Comment 2•12 years ago
|
||
I would like to take this up.
Comment 5•12 years ago
|
||
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)
Comment 6•12 years ago
|
||
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 7•12 years ago
|
||
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.
Assignee | ||
Comment 9•11 years ago
|
||
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)
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #766383 -
Flags: review?(evilpies)
Assignee | ||
Comment 11•11 years ago
|
||
Same as v1, but a few more tests.
Attachment #766383 -
Attachment is obsolete: true
Attachment #766383 -
Flags: review?(evilpies)
Attachment #766389 -
Flags: review?(evilpies)
Assignee | ||
Comment 12•11 years ago
|
||
This isn't quite finished. I need to change the type of the loop variable from uint32_t to double. Good night!
Assignee | ||
Comment 13•11 years ago
|
||
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)
Reporter | ||
Comment 14•11 years ago
|
||
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+
Reporter | ||
Comment 15•11 years ago
|
||
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+
Assignee | ||
Comment 16•11 years ago
|
||
(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.
Comment 17•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/98dccff45810
https://hg.mozilla.org/mozilla-central/rev/d0c3168c3c47
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Assignee | ||
Comment 18•11 years ago
|
||
I went with assertDeepEq, btw.
Updated•11 years ago
|
Keywords: dev-doc-needed
Comment 19•11 years ago
|
||
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)`.
Reporter | ||
Comment 20•11 years ago
|
||
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 → ---
Updated•11 years ago
|
relnote-firefox:
--- → ?
Updated•11 years ago
|
Assignee | ||
Comment 21•11 years ago
|
||
In a new bug please?
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
relnote-firefox:
25+ → ---
Resolution: --- → FIXED
Comment 22•11 years ago
|
||
Updated•11 years ago
|
Summary: Implement ES6 Array.from and Array.of → Implement ES6 Array.of
Comment 23•11 years ago
|
||
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
Comment 24•11 years ago
|
||
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.
Comment 25•11 years ago
|
||
(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 :)
Comment 26•11 years ago
|
||
> (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
Comment 27•11 years ago
|
||
(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
Comment 28•11 years ago
|
||
Perfect! Thanks a lot.
Sebastian
Comment 29•11 years ago
|
||
Florian reviewed the page. Thanks all and especially rick who wrote it!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•