Closed Bug 1107645 Opened 5 years ago Closed 5 years ago

Implement %TypedArray%.prototype.forEach

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: evilpie, Assigned: tephra, Mentored)

References

Details

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

Attachments

(1 file, 5 obsolete files)

+++ 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
Mentor: evilpies
I would like to take this one.
No longer blocks: 1075184
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.
Assignee: nobody → mozbugs.retornam
Attached patch bug-1107645.patch without tests (obsolete) — Splinter Review
Attachment #8535699 - Flags: review?(evilpies)
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".
Attachment #8535699 - Flags: review?(evilpies)
Attached patch bug-1107645-v2.patch (obsolete) — Splinter Review
Attachment #8535699 - Attachment is obsolete: true
Attachment #8535715 - Flags: review?(evilpies)
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
Attachment #8535715 - Flags: review?(evilpies)
raymond are you still interested in this?
Assignee: mozbugs.retornam → 446240525
Assignee: 446240525 → mozbugs.retornam
Yes I am still interested and will submit a patch soon. Sorry for the delay
Raymond, do you know when you can submit a new patch?
Clearing bug assignment, everyone who wants can work on this again. Have fun! :)
Assignee: mozbugs.retornam → nobody
Attached patch bug1107645-eric-skoglund.patch (obsolete) — Splinter Review
Would like some pointers on implementing some tests and as such this patch doesn't include any test.
Attachment #8553071 - Flags: review?(evilpies)
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.
Attachment #8553071 - Flags: review?(evilpies)
Attached patch bug1107645v2EricSkoglund.patch (obsolete) — Splinter Review
Attachment #8553071 - Attachment is obsolete: true
Attachment #8554394 - Flags: review?(evilpies)
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(), {})
Attachment #8554394 - Flags: review?(evilpies)
Attached patch bug1107645v3-eric-skoglund.patch (obsolete) — Splinter Review
Hopefully I fixed everything requested.
Attachment #8554394 - Attachment is obsolete: true
Attachment #8554645 - Flags: review?(evilpies)
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
Attachment #8554645 - Flags: review?(evilpies) → review+
Attachment #8535715 - Attachment is obsolete: true
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(), {})
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?
No this should actually be fixed now, try again?
Attachment #8554645 - Attachment is obsolete: true
Attachment #8554735 - Flags: review?(evilpies)
(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.
Attachment #8554735 - Flags: review?(evilpies) → review+
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1d541c37cb8
Assignee: nobody → eric
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3dbbf3baf9f0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.