Implement %TypedArray%.prototype.forEach

👁 Image	Tom S. (please needinfo tschuster) Reporter
	Description • 11 years ago

+++ 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

👁 Image	Tom S. (please needinfo tschuster) Reporter
	Updated • 11 years ago

👁 Image	Eric Skoglund [:tephra] Assignee
	Comment 1 • 11 years ago

I would like to take this one.

👁 Image	Tom S. (please needinfo tschuster) Reporter
	Updated • 11 years ago

👁 Image	Tom S. (please needinfo tschuster) Reporter
	Comment 2 • 11 years ago

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.

👁 Image	raymond [:retornam] (inactive)
	Updated • 11 years ago

👁 Image	raymond [:retornam] (inactive)
	Comment 3 • 11 years ago

👁 Image	Tom S. (please needinfo tschuster) Reporter
	Comment 4 • 11 years ago

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".

👁 Image	raymond [:retornam] (inactive)
	Comment 5 • 11 years ago

👁 Image	Tom S. (please needinfo tschuster) Reporter
	Comment 6 • 11 years ago

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

👁 Image	Tom S. (please needinfo tschuster) Reporter
	Updated • 11 years ago

👁 Image	Tom S. (please needinfo tschuster) Reporter
	Comment 7 • 11 years ago

raymond are you still interested in this?

👁 Image	ziyunfei
	Updated • 11 years ago

👁 Image	raymond [:retornam] (inactive)
	Updated • 11 years ago

👁 Image	raymond [:retornam] (inactive)
	Comment 8 • 11 years ago

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

👁 Image	Tom S. (please needinfo tschuster) Reporter
	Comment 9 • 11 years ago

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

👁 Image	Tom S. (please needinfo tschuster) Reporter
	Comment 10 • 11 years ago

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

👁 Image	Eric Skoglund [:tephra] Assignee
	Comment 11 • 11 years ago

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

👁 Image	Tom S. (please needinfo tschuster) Reporter
	Comment 12 • 11 years ago

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.

👁 Image	Eric Skoglund [:tephra] Assignee
	Comment 13 • 11 years ago

👁 Image	Tom S. (please needinfo tschuster) Reporter
	Comment 14 • 11 years ago

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(), {})

👁 Image	Eric Skoglund [:tephra] Assignee
	Comment 15 • 11 years ago

Hopefully I fixed everything requested.

👁 Image	Tom S. (please needinfo tschuster) Reporter
	Comment 16 • 11 years ago

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

👁 Image	Tom S. (please needinfo tschuster) Reporter
	Updated • 11 years ago

👁 Image	Tom S. (please needinfo tschuster) Reporter
	Comment 17 • 11 years ago

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(), {})

👁 Image	Eric Skoglund [:tephra] Assignee
	Comment 18 • 11 years ago

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?

👁 Image	Tom S. (please needinfo tschuster) Reporter
	Comment 19 • 11 years ago

No this should actually be fixed now, try again?

👁 Image	Eric Skoglund [:tephra] Assignee
	Comment 20 • 11 years ago

👁 Image	Eric Skoglund [:tephra] Assignee
	Comment 21 • 11 years ago

(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.

👁 Image	Tom S. (please needinfo tschuster) Reporter
	Updated • 11 years ago

👁 Image	Till Schneidereit [:till]
	Comment 22 • 11 years ago

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

👁 Image	Eric Skoglund [:tephra] Assignee
	Updated • 11 years ago

👁 Image	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 11 years ago

👁 Image	Carsten Book [:Tomcat]
	Comment 24 • 11 years ago

👁 Image	Florian Scholz (Open Web Docs)
	Comment 25 • 11 years ago

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