[ES6] Implement Array.prototype.copyWithin
Created attachment 253657 [details] Patch
Comment on attachment 253657 [details] Patch Attachment 253657 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5513774047952896 New failing tests: js/Object-getOwnPropertyNames.html
Created attachment 253659 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 253660 [details] Patch
Comment on attachment 253660 [details] Patch Attachment 253660 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5740288073007104 New failing tests: js/Object-getOwnPropertyNames.html
Created attachment 253661 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 253662 [details] Patch
Comment on attachment 253662 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253662&action=review Added comment for ease of review > Source/JavaScriptCore/builtins/Array.prototype.js:468 > + } This function doesn't contain negative zeros. > Source/JavaScriptCore/builtins/Array.prototype.js:472 > + } This function may take a negative zero as a first argument. And positive is always >= +0. So we can ensure that when the result of this function is 0, it's positive.
Comment on attachment 253662 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253662&action=review > Source/JavaScriptCore/builtins/Array.prototype.js:463 > +function copyWithin(target, start) { Please put function start braces on a new line to match WebKit coding style practice. > Source/JavaScriptCore/builtins/Array.prototype.js:475 > + if (thisValue === null || thisValue === undefined) I don’t understand the use of "undefined" here. I don’t think there is a keyword "undefined". > Source/JavaScriptCore/builtins/Array.prototype.js:490 > + if (end === undefined) I don’t understand the use of "undefined" here. I don’t think there is a keyword "undefined". > Source/JavaScriptCore/builtins/Array.prototype.js:511 > + var fromValue = thisObject[from]; > + thisObject[to] = fromValue; Why the var? Why not just direct assignment?
Comment on attachment 253662 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253662&action=review Thanks. I'll upload the revised patch soon. >> Source/JavaScriptCore/builtins/Array.prototype.js:463 >> +function copyWithin(target, start) { > > Please put function start braces on a new line to match WebKit coding style practice. Fixed. >> Source/JavaScriptCore/builtins/Array.prototype.js:475 >> + if (thisValue === null || thisValue === undefined) > > I don’t understand the use of "undefined" here. I don’t think there is a keyword "undefined". Nice question! According to the spec, `undefined` global variable is defined as `writable: false` and `configurable: false`. (http://people.mozilla.org/~jorendorff/es6-draft.html#sec-undefined) And actually, JSC implements so. As the result, users cannot replace / overwrite the `undefined` global variable. So it is ensured that this reference is safe (it always indicates `undefined` value). >> Source/JavaScriptCore/builtins/Array.prototype.js:490 >> + if (end === undefined) > > I don’t understand the use of "undefined" here. I don’t think there is a keyword "undefined". I've noted the reason in the above comment. >> Source/JavaScriptCore/builtins/Array.prototype.js:511 >> + thisObject[to] = fromValue; > > Why the var? Why not just direct assignment? Using direct assignment does not have any problem. fixed.
Created attachment 253708 [details] Patch
Comment on attachment 253708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253708&action=review I’d like to see performance tests for this in the future. I am surprised, for example, that using a single loop for both directions is sufficiently efficient. > Source/JavaScriptCore/builtins/Array.prototype.js:505 > + if (from < to && to < (from + count)) { I probably would not have used parentheses here.
Comment on attachment 253708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253708&action=review Thanks for your quick reply! If some application uses it (maybe WebGL user will use it for moving memory representation), we can adopt it into jetstream! (and check the performance) >> Source/JavaScriptCore/builtins/Array.prototype.js:505 >> + if (from < to && to < (from + count)) { > > I probably would not have used parentheses here. I'll fix it :D
Committed r184863: <http://trac.webkit.org/changeset/184863>
Comment on attachment 253708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253708&action=review Post review comments. Just nits > Source/JavaScriptCore/builtins/Array.prototype.js:463 > +function copyWithin(target, start) Nit: You should probably add your copyright to the top of this file, as this is a non-trivial addition: * Copyright (C) 2015 Yusuke Suzuki <utatane.tea@gmail.com>. Nit: Other functions in this file have been mentioning the optional arguments in comments. That might be nice here: copyWithin(target, start /*, end */) > Source/JavaScriptCore/builtins/Array.prototype.js:477 > + var thisValue = this; Hmm, is this variable necessary? It is only used 3 times below, and I'd think just using "this" would be clear. The other functions up above do not use this style. > Source/JavaScriptCore/builtins/Array.prototype.js:479 > + throw new @TypeError("Array.copyWithin requires that |this| be not null or undefined"); Nit: "be not" is unusual phrasing. "not be" would be better. > Source/JavaScriptCore/tests/stress/array-copywithin.js:38 > +// 1 arguments. How about a test for 0 arguments.
Comment on attachment 253708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253708&action=review Great post review! I'll fix it in the separate patch (and is it better to upload it to this issue?) >> Source/JavaScriptCore/builtins/Array.prototype.js:463 >> +function copyWithin(target, start) > > Nit: You should probably add your copyright to the top of this file, as this is a non-trivial addition: > > * Copyright (C) 2015 Yusuke Suzuki <utatane.tea@gmail.com>. > > Nit: Other functions in this file have been mentioning the optional arguments in comments. That might be nice here: > > copyWithin(target, start /*, end */) OK, I'll add it :) & fix this. >> Source/JavaScriptCore/builtins/Array.prototype.js:477 >> + var thisValue = this; > > Hmm, is this variable necessary? It is only used 3 times below, and I'd think just using "this" would be clear. The other functions up above do not use this style. make sense. I followed to the C++ style (first taking JSCValue thisValue, check it and cast it to thisObject). But it's not necessary in builtins js code. >> Source/JavaScriptCore/builtins/Array.prototype.js:479 >> + throw new @TypeError("Array.copyWithin requires that |this| be not null or undefined"); > > Nit: "be not" is unusual phrasing. "not be" would be better. I'll fix it. In the same patch, I'll fix StringIterator.prototype.js about the same issue. >> Source/JavaScriptCore/tests/stress/array-copywithin.js:38 >> +// 1 arguments. > > How about a test for 0 arguments. Nice catch! I'll add it.
Reopening to attach new patch.
Created attachment 253725 [details] Patch
Comment on attachment 253725 [details] Patch thank you for your review ;)
FWIW, Geoff suggested not defining functions within the prototype implementations. I don't remember the reason unfortunately.
(In reply to comment #20) > FWIW, Geoff suggested not defining functions within the prototype > implementations. I don't remember the reason unfortunately. Thank you for your notice! It seems that Array.prototype.sort already has functions. Previously, defining functions inside builtins is prohibited by assertion. But I remember that I relaxed this restriction in Array.from patch. Geoff, do you have any thought about it?
Comment on attachment 253725 [details] Patch Clearing flags on attachment: 253725 Committed r184871: <http://trac.webkit.org/changeset/184871>
All reviewed patches have been landed. Closing bug.