1107645 - Implement %TypedArray%.prototype.forEach

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Tom S]

+++ This bug was initially created as a clone of Bug #1107601 +++

This is specified at:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-%typedarray%.prototype.foreach
You can look at Bug #1078975 to get a general idea how this works.

So what you have to do is:
- Take a look at Array.js and copy the code for forEach to TypedArray.js
- Add the code for "This function is not generic" like it's done in TypedArrayFind.
- Add a reference to the functions in TypedArray.cpp in the TypedArrayObject::protoFunctions array. You can take "find" as a reference again.
- Add the function to the array in test_xrayToJS.xul, like it's done for Bug 1078975.
- Write some tests

Comment 1

[Eric Skoglund [:tephra]]

I would like to take this one.

Comment 2

[Tom S]

Feel free to start working on this.

If you have any questions, ask here or join us in the #jsapi channel on irc.mozilla.org.

Comment 3

[raymond [:retornam] (inactive)]

Attachment: bug-1107645.patch without tests

Comment 4

[Tom S]

Comment on attachment 8535699 [details] [diff] [review]
bug-1107645.patch without tests

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

Good first patch! Are you interested in working on the tests?

::: js/src/builtin/TypedArray.js
@@ +79,5 @@
> +
> +function TypedArrayForEach(callbackfn/*, thisArg*/) {
> +    // This function is not generic.
> +    if (!IsObject(this) || !IsTypedArray(this)) {
> +        return callFunction(CallTypedArrayMethodIfWrapped, this, predicate, thisArg,

This has to be callFunction(CallTypedArrayMethodIfWrapped, this, callbackfn, thisArg, "TypedArrayForEach");

@@ +82,5 @@
> +    if (!IsObject(this) || !IsTypedArray(this)) {
> +        return callFunction(CallTypedArrayMethodIfWrapped, this, predicate, thisArg,
> +                            "TypedArrayForEach");
> +    }
> +    /* Step 1. */

Please change those from /* Step 1. */ to // Step 1.
Do the same for all other comments.

@@ +83,5 @@
> +        return callFunction(CallTypedArrayMethodIfWrapped, this, predicate, thisArg,
> +                            "TypedArrayForEach");
> +    }
> +    /* Step 1. */
> +    var O = ToObject(this);

As I explained this has to var = this;

@@ +86,5 @@
> +    /* Step 1. */
> +    var O = ToObject(this);
> +
> +    /* Steps 2-3. */
> +    var len = TO_UINT32(O.length);

This has to be
var len = TypedArrayLength(O);

@@ +101,5 @@
> +    /* Steps 6-7. */
> +    /* Steps a (implicit), and d. */
> +    for (var k = 0; k < len; k++) {
> +        /* Step b */
> +        if (k in O) {

This is not obvious, but we can omit this if.

i.e consider code like
x = new Int8Array(3)
'0 in x', '1 in x' and '2 in x' are always true.
There are no "holes".

Comment 5

[raymond [:retornam] (inactive)]

Attachment: bug-1107645-v2.patch

Comment 6

[Tom S]

Comment on attachment 8535715 [details] [diff] [review]
bug-1107645-v2.patch

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

::: js/src/builtin/TypedArray.js
@@ +82,5 @@
> +    if (!IsObject(this) || !IsTypedArray(this)) {
> +        return callFunction(CallTypedArrayMethodIfWrapped, this, callbackfn, thisArg,
> +                            "TypedArrayForEach");
> +    }
> +    //Step 1.

Please put a space between // Step here and everywhere else.

If you look at https://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.foreach, you might notice that the numbers changed. Would you like to these to the new step numbers? 
Instead of Step 1. you would have Steps 1-2. for example.

@@ +83,5 @@
> +        return callFunction(CallTypedArrayMethodIfWrapped, this, callbackfn, thisArg,
> +                            "TypedArrayForEach");
> +    }
> +    //Step 1.
> +    var  = this;

What about the var >>> O <<< = this?

@@ +98,5 @@
> +    //Step 5.
> +    var T = arguments.length > 1 ? arguments[1] : void 0;
> +
> +    //Steps 6-7
> +    for (var k = 0; k < len; k++) {

I would add a comment here like // Step 9 a-c. are unnecessary, the condition is always true for TypedArrays

Comment 7

[Tom S]

raymond are you still interested in this?

Comment 8

[raymond [:retornam] (inactive)]

Yes I am still interested and will submit a patch soon. Sorry for the delay

Comment 9

[Tom S]

Raymond, do you know when you can submit a new patch?

Comment 10

[Tom S]

Clearing bug assignment, everyone who wants can work on this again. Have fun! :)

Comment 11

[Eric Skoglund [:tephra]]

Attachment: bug1107645-eric-skoglund.patch

Would like some pointers on implementing some tests and as such this patch doesn't include any test.

Comment 12

[Tom S]

Comment on attachment 8553071 [details] [diff] [review]
bug1107645-eric-skoglund.patch

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

Thank you for picking this up, it already looks quite good.

Have you tried to test this code already? i.e manually by executing something like (new Uint8Array(3)).forEach?

So on how to test this. Have a look at tests/ecma_6/TypedArray/every-and-some.js. First you should copy most of the tests that are for "some" into a new file "forEach.js". Then you have to adjust the test to make sense for forEach. This means you can actually remove a few tests, because forEach doesn't have any return value.

You can run your test with jstests.py {path}/dist/bin/js ecma_6/TypedArray/forEach.

Don't hesitate to ask if you have any more questions, you might want to check out the #jsapi channel on irc.mozilla.org.

::: js/src/builtin/TypedArray.js
@@ +63,5 @@
>  function TypedArrayFill(value, start = 0, end = undefined) {
>      // This function is not generic.
>      if (!IsObject(this) || !IsTypedArray(this)) {
>          return callFunction(CallTypedArrayMethodIfWrapped, this, value, start, end,
> +                       n     "TypedArrayFill");

"n" has to go.

@@ +173,5 @@
> +// ES6 draft rev30 (2014-12-24) 22.2.3.12 %TypedArray%.prototype.forEach(callbackfn[,thisArg])
> +function TypedArrayForEach(callbackfn, thisArg = undefined) {
> +    // This function is not generic
> +    if (!IsObject(this) || !IsTypedArray(this)) {
> +	return callfunction(CallTypedArrayMethodIfWrapped, this, predicate, thisArg,

This is not quite correct. It's callFunction instead of callfunction.

As for the arguments, the general idea of how callFunction works is that everything after CallTypedArrayMethodIfWrapped, are this used to invoke the function and then all parameters after that are passed on to the function.

So this is like CallTypedArrayMethodIfWrapped.call(this, predicate, thisArg, "TypedArrayForEach") in normal JS code. Notice that predicate should be callbackfn.

@@ +176,5 @@
> +    if (!IsObject(this) || !IsTypedArray(this)) {
> +	return callfunction(CallTypedArrayMethodIfWrapped, this, predicate, thisArg,
> +			   "TypedArrayForEach");
> +    }
> +    

Whitespace

@@ +177,5 @@
> +	return callfunction(CallTypedArrayMethodIfWrapped, this, predicate, thisArg,
> +			   "TypedArrayForEach");
> +    }
> +    
> +    // Step 1-2

Please add a dot after 2 and all other step comments.

Comment 13

[Eric Skoglund [:tephra]]

Attachment: bug1107645v2EricSkoglund.patch

Comment 14

[Tom S]

Comment on attachment 8554394 [details] [diff] [review]
bug1107645v2EricSkoglund.patch

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

Thank you this already looks really solid, especially also the test.
You have to adjust js/xpconnect/tests/chrome/test_xrayToJS.xul and add "forEach" to gPrototypeProperties['TypedArray'].
I would like to see one more version of this where you change forEach to follow revision 31.

::: js/src/builtin/TypedArray.js
@@ +169,5 @@
>      // Step 10.
>      return -1;
>  }
>  
> +// ES6 draft rev30 (2014-12-24) 22.2.3.12 %TypedArray%.prototype.forEach(callbackfn[,thisArg])

Please look at rev31 and change the steps to follow that spec.

@@ +176,5 @@
> +    if (!IsObject(this) || !IsTypedArray(this)) {
> +	return callFunction(CallTypedArrayMethodIfWrapped, this, callbackfn, thisArg,
> +			    "TypedArrayForEach");
> +    }
> +    

There is still whitespace here, you might want to configure your editor to show trailing spaces.

@@ +183,5 @@
> +
> +    // Step 3-5.
> +    var len = TypedArrayLength(O);
> +
> +    var T = arguments.length > 1 ? arguments[1] : void 0;

This can just be T = thisArg, because thisArg is has a default parameter of undefined. This should be step 6.

@@ +186,5 @@
> +
> +    var T = arguments.length > 1 ? arguments[1] : void 0;
> +    
> +    // Step 6-7.
> +    if (arguments.length === 0)

I am looking at the working draft rev31 15-1-15 and this is actually step 5 and before var T.

@@ +197,5 @@
> +    for (var k = 0; k < len; k++) {
> +	// Step 9c-9d are unnecessary since the condition always holds true for TypedArray.
> +	// Step 9c.
> +	callFunction(callbackfn, T, O[k], k, 0);
> +    }

Let's be explicit about it
// Step 9.
return undefined;

::: js/src/tests/ecma_6/TypedArray/forEach.js
@@ +62,5 @@
> +        thrown = true;
> +    }
> +    assertEq(thrown, true);
> +    assertEq(sum, 6);
> +    assertEq(count, 3);

Please add one test where you check that the return value of forEach is undefined.

@@ +91,5 @@
> +        assertEq(sum, 6);
> +    }
> +
> +    // Throws if `this` isn't a TypedArray.
> +    var invalidReceivers = [undefined, null, 1, false, "", Symbol(), [], {}, /./];

one more please new Proxy(new constructor(), {})

Comment 15

[Eric Skoglund [:tephra]]

Attachment: bug1107645v3-eric-skoglund.patch

Hopefully I fixed everything requested.

Comment 16

[Tom S]

Comment on attachment 8554645 [details] [diff] [review]
bug1107645v3-eric-skoglund.patch

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

Well done, thanks.

::: js/src/builtin/TypedArray.js
@@ +169,5 @@
>      // Step 10.
>      return -1;
>  }
>  
> +// ES6 draft rev3 (2015-01-15) 22.1.3.10 %TypedArray%.prototype.forEach(callbackfn[,thisArg])

rev31

@@ +200,5 @@
> +	// Step 8d.
> +	callFunction(callbackfn, T, O[k], k, O);
> +    }
> +
> +    // Step 9.    

whitespace

Comment 17

[Tom S]

Comment on attachment 8554645 [details] [diff] [review]
bug1107645v3-eric-skoglund.patch

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

Well done, thanks.

::: js/src/tests/ecma_6/TypedArray/forEach.js
@@ +91,5 @@
> +        assertEq(sum, 6);
> +    }
> +
> +    // Throws if `this` isn't a TypedArray.
> +    var invalidReceivers = [undefined, null, 1, false, "", Symbol(), [], {}, /./];

Don't forget new Proxy(new constructor(), {})

Comment 18

[Eric Skoglund [:tephra]]

Hmm if I add 'new Proxy(new constructor(), {})' proxy to the invalidReceivers array I get a failure that no TypeError is thrown. Is this due to bug 1115361?

Comment 19

[Tom S]

No this should actually be fixed now, try again?

Comment 20

[Eric Skoglund [:tephra]]

Attachment: bug1107645v4.patch

Comment 21

[Eric Skoglund [:tephra]]

(In reply to Tom Schuster [:evilpie] from comment #19)
> No this should actually be fixed now, try again?

Yes, sorry I had missed to merge into my branch.

Comment 22

[Till Schneidereit [:till]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1d541c37cb8

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dbbf3baf9f0

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3dbbf3baf9f0

Comment 25

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/38#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/forEach