Closed Bug 896608 Opened 11 years ago Closed 9 years ago

Implement ES6 %TypedArray%.{of, from}

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: bbenvie, Assigned: 446240525)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(1 file, 2 obsolete files)

These are nearly equivalent to `Array.of` and `Array.from`, but have a few subtle distinctions:

* `%TypedArray%.{of, from}` throws if `IsConstructor(this)` is false, where `Array.{of, from}` defaults to creating a new Array.
* `%TypedArray%.{of, from}` uses `Put` where `Array.{of, from}` use `DefinePropertyOrThrow`.
* When `from` gets an iterator, the %TypedArray% version first collects all the values from the iterator, then creates an instance of |this| using the count, then sets the values on the instance. Array.from set each value as it gets them from the iterator then sets the length at the end.
* When `Array.from` gets an arraylike which isn't an iterator, it respects holes, where `%TypedArray%.from` will ensure the result is dense.

All of these things are observable from user code, so they need to be implemented. I'm sure it's possible to make them a lot more efficient if the receiver is a real TypedArray constructor, but the spec makes it explicit that these static methods are intentionally generic and can work with any constructable |this| value.
Summary: Implemented ES6 %TypedArray%.{of, from} → Implement ES6 %TypedArray%.{of, from}
See ES6 spec draft (July 2013 revision) sections 15.13.6.2.2 and 15.13.6.2.3.
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Assignee: general → nobody
Assignee: nobody → 446240525
Blocks: 1071668
No longer blocks: es6
Component: JavaScript Engine → JavaScript: Standard Library
Attached patch bug-896608-v1.patch (obsolete) — Splinter Review
Tests for %TypedArray%.from are adapted from the tests for Array.from.
Attachment #8547040 - Flags: review?(evilpies)
Comment on attachment 8547040 [details] [diff] [review]
bug-896608-v1.patch

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

That's a lot tests. \o/ I think it would be better if Till also took a look at this. I have been reviewing all your patches a new set of eyes on such a big one can't hurt.

::: js/src/builtin/TypedArray.js
@@ +499,5 @@
> +    var C = this;
> +
> +    // Step 2.
> +    if (!IsConstructor(C))
> +        ThrowError(JSMSG_NOT_CONSTRUCTOR, DecompileArg(1, C));

See comment to of()

@@ +525,5 @@
> +    var mapping = mapfn !== undefined;
> +    var T = thisArg;
> +
> +    // Steps 8-9.
> +    var usingIterator = ToObject(items)[std_iterator];

This doesn't really implement the whole GetMethod semantics. You should probably implement that in Utilities.js

@@ +530,5 @@
> +
> +    // Step 10.
> +    if (usingIterator !== undefined) {
> +        // Steps 10.a-b.
> +        var iterator = callFunction(usingIterator, items);

GetIterator needed here as well.

@@ +533,5 @@
> +        // Steps 10.a-b.
> +        var iterator = callFunction(usingIterator, items);
> +
> +        // Step 10.c.
> +        var values = [];

I think you should use "new List" here.

@@ +605,5 @@
> +function TypedArrayStaticOf(...items) {
> +    // Step 1.
> +    var len = items.length;
> +
> +    // Steps 2(implicit, achieved by the rest argument)-3.

// Step 2 (implicit)
// Step 3

 seems somehow nicer

@@ +610,5 @@
> +    var C = this;
> +
> +    // Steps 4-5.
> +    if (IsConstructor(C))
> +        var newObj = new C(len);

Reverse this.

if (!IsConstructor(C)
  throw

newObj = new C(len)

@@ +612,5 @@
> +    // Steps 4-5.
> +    if (IsConstructor(C))
> +        var newObj = new C(len);
> +    else
> +        ThrowError(JSMSG_NOT_CONSTRUCTOR, DecompileArg(1, C));

DecompileArg is wrong. C is not an argument at all. For now typeof C is probably good enough.
Attachment #8547040 - Flags: review?(evilpies) → review?(till)
Comment on attachment 8547040 [details] [diff] [review]
bug-896608-v1.patch

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

Very, very nice. As always.

I did add quite a few comments which need to be addressed, but they're all pretty straight-forward, so r=me with that done.

::: js/src/builtin/TypedArray.js
@@ +512,5 @@
> +    // Step 5.
> +    var t = thisArg;
> +
> +    // Step 6.
> +    return TypedArrayFrom(C, undefined, source, f, t);

Personally, I'd just inline `t` and make it "// Steps 5-6." I don't much care either way, though.

@@ +518,5 @@
> +
> +// ES6 draft rev30 (2014/12/24) 22.2.2.1.1 TypedArrayFrom().
> +function TypedArrayFrom(constructor, target, items, mapfn, thisArg) {
> +    // Step 1.
> +    var C = constructor;

I think it actually wouldn't hurt to implement the asserts from steps 2-5. We have `assert` available for self-hosted code, so doing so is simple enough.

@@ +522,5 @@
> +    var C = constructor;
> +
> +    // Steps 6-7.
> +    var mapping = mapfn !== undefined;
> +    var T = thisArg;

Nice, I like that you chose to simplify the implementation here.

@@ +525,5 @@
> +    var mapping = mapfn !== undefined;
> +    var T = thisArg;
> +
> +    // Steps 8-9.
> +    var usingIterator = ToObject(items)[std_iterator];

I agree. AFAICT, there's only one tiny visible change in behavior, but it's still useful to have this as a utility function.

The behavioral change is that step 10.a will throw an exception about `null` not being callable if `items[@@iterator]` is `null`. According to GetMethod step 4, it should be changed to `undefined`. Add a test for this (admittedly almost ridiculous) corner case.

@@ +533,5 @@
> +        // Steps 10.a-b.
> +        var iterator = callFunction(usingIterator, items);
> +
> +        // Step 10.c.
> +        var values = [];

Yes ...

@@ +535,5 @@
> +
> +        // Step 10.c.
> +        var values = [];
> +
> +        // Steps 10.d-e.

The step numbers are off beginning here. This should be d-f, with the following numbering fixed accordingly.

@@ +544,5 @@
> +                ThrowError(JSMSG_NEXT_RETURNED_PRIMITIVE);
> +
> +            // Steps 10.e.iii-vi.
> +            if (next.done !== true) {
> +                var nextValue = next.value;

Inline `nextValue` and get rid of the braces. I also wouldn't be opposed to inverting this and getting rid of the `else`, too:

if (next.done)
    break;
values.push(next.value);

(As it turns out, this is even slightly more correct: your version does a strict comparison between `next.done` and a boolean. If the iterator is overridden by content, `next.done` might not be a boolean at all. I guess that's why the spec uses `next` as the result of `ToBoolean(result.done)`. Hence, if you choose not to invert the condition, at least change it to `if (!next.done)`.)

@@ +545,5 @@
> +
> +            // Steps 10.e.iii-vi.
> +            if (next.done !== true) {
> +                var nextValue = next.value;
> +                values.push(nextValue);

Not using List in 10.c makes content overriding `Array.prototype.push` break this.

@@ +571,5 @@
> +            // Steps 10.j.v-vi.
> +            targetObj[k] = mappedValue;
> +        }
> +
> +        // Step 10.l.

Add (the real, after the numbering is fixed) step l here as a comment along the lines of "asserting that `values` is empty here would require removing them one by one from the list's start in the loop above. That would introduce unacceptable overhead. Additionally, the loop's logic is simple enough not to require the assert."

@@ +573,5 @@
> +        }
> +
> +        // Step 10.l.
> +        return targetObj;
> +    } else {

Given that not even the spec uses an `else` here, just get rid of it and un-indent all following code one level.

@@ +574,5 @@
> +
> +        // Step 10.l.
> +        return targetObj;
> +    } else {
> +        // Steps 12-13.

Copy over the comment about step 11 from ArrayFrom. (It's for step 9 in that case.)

@@ +581,5 @@
> +        // Steps 14-16.
> +        var len = ToLength(arrayLike.length);
> +
> +        // Steps 17-18.
> +        var targetObj = new C(len);

Add a comment "// See comment for steps i-h."

@@ +601,5 @@
> +    }
> +}
> +
> +// ES6 draft rev30 (2014/12/24) 22.2.2.2 %TypedArray%.of(...items).
> +function TypedArrayStaticOf(...items) {

While I really like using rest args here, they're somewhat slower than `arguments`, at least for now. Given how simple the code stays if `arguments` is used, I'd slightly prefer that.

@@ +605,5 @@
> +function TypedArrayStaticOf(...items) {
> +    // Step 1.
> +    var len = items.length;
> +
> +    // Steps 2(implicit, achieved by the rest argument)-3.

Either that, or "// Steps 2 (implicit) and 3.". (But see the above comment about using `arguments`)

@@ +610,5 @@
> +    var C = this;
> +
> +    // Steps 4-5.
> +    if (IsConstructor(C))
> +        var newObj = new C(len);

(Obviously still `var newObject =`.)

@@ +615,5 @@
> +    else
> +        ThrowError(JSMSG_NOT_CONSTRUCTOR, DecompileArg(1, C));
> +
> +    // Steps 6-7.
> +    for (var k = 0; k < len; k++) {

nit: no braces necessary.

::: js/src/tests/ecma_6/TypedArray/from_basics.js
@@ +7,5 @@
> +    Int32Array,
> +    Uint32Array,
> +    Float32Array,
> +    Float64Array
> +];

Please test that `constructor.from` is identical for all constructors. Something like `let fromMethod = constructors[0].from; for (let i = 1; i < constructors.length; i++) assertEq(constructors[i], fromMethod` should work.

@@ +33,5 @@
> +    assertDeepEq(constructor.from(src), new constructor([0, 1]));
> +
> +    // If an object has neither an @@iterator method nor .length,
> +    // then it's treated as zero-length.
> +    assertDeepEq(constructor.from({}), new constructor);

style nit: please still use () for argument-less constructor calls. Here and everywhere else.

::: js/src/tests/ecma_6/TypedArray/from_constructor.js
@@ +42,5 @@
> +
> +    // Note %TypedArray%.from(iterable) calls 'this' with an argument whose value is
> +    // `[...iterable].length`, while Array.from(iterable) doesn't pass any argument.
> +    assertEq(constructor.from.call(Object, []) instanceof Number, true);
> +    assertEq(Array.from.call(Object, []) instanceof Object, true);

Please go through the tests and remove the ones that don't apply to TypedArray.from. This is one example, but there are many more.

@@ +71,5 @@
> +        }, TypeError);
> +    }
> +
> +    // constructor.from does not get confused if global.Array is replaced with another
> +    // constructor.

This entire test should be changed to work with `constructor`. You can probably do something like `var RealConstructor = constructor; this[constructor.name] = NotTypedArray`, etc.

::: js/src/tests/ecma_6/TypedArray/from_proxy.js
@@ +37,5 @@
> +    }
> +
> +    // Unlike Array.from, when the new object created by %TypedArray%.from is a Proxy,
> +    // %TypedArray%.from calls handler.set to create new elements rather than handler.defineProperty,
> +    // and %TypedArray%.from doesn't set the length property at the end.

Perhaps something like "Unlike Array.from, %TypedArray%.from uses [[Put]] instead of [[DefineOwnProperty]]. Hence, it calls handler.set to create new elements rather than handler.defineProperty. Additionally, it doesn't set the `length` property at the end."

::: js/src/tests/ecma_6/TypedArray/from_string.js
@@ +9,5 @@
> +    Float32Array,
> +    Float64Array
> +];
> +
> +for (var constructor of constructors) {

It doesn't make sense to run these tests for all typed array types: the `from` method is always identical and the constructor is never used as the call receiver, so there can't be any behavioral differences.

::: js/src/tests/ecma_6/TypedArray/of.js
@@ +68,5 @@
> +    assertEq(constructor.of.call(function*(len) {
> +        return len;
> +    }, "a", "b", "c").next().value, 3);
> +
> +    // An exception might be thrown in a strict assignment to the new object's indexed properties. 

Nit: trailing whitespace
Attachment #8547040 - Flags: review?(till) → review+
Blocks: 1121391
(In reply to Till Schneidereit [:till] from comment #5)
> @@ +535,5 @@
> > +
> > +        // Step 10.c.
> > +        var values = [];
> > +
> > +        // Steps 10.d-e.
> 
> The step numbers are off beginning here. This should be d-f, with the
> following numbering fixed accordingly.

Till, please see comment 2.
(In reply to ziyunfei from comment #6)
> Till, please see comment 2.

Oh, I see. Ignore the comments about that and adapt the steps I mentioned in my comments accordingly, then.
Attached patch bug-889158-v2.patch (obsolete) — Splinter Review
Attachment #8547040 - Attachment is obsolete: true
Attachment #8549033 - Flags: review?(till)
Attachment #8549033 - Attachment is obsolete: true
Attachment #8549033 - Flags: review?(till)
Attachment #8549037 - Flags: review?(till)
Comment on attachment 8549037 [details] [diff] [review]
remove trailing whitespaces

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

Very nice, thanks!
Attachment #8549037 - Flags: review?(till) → review+
https://hg.mozilla.org/mozilla-central/rev/828b434f69f7
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
See Also: → 1122396
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: