Bug 145107 – [ES6] Implement Array.prototype.copyWithin

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[ES6] Implement Array.prototype.copyWithin

[Yusuke Suzuki]

Created attachment 253657 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Yusuke Suzuki]

Created attachment 253660 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Yusuke Suzuki]

Created attachment 253662 [details]
Patch

[Yusuke Suzuki]

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.

[Darin Adler]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 253708 [details]
Patch

[Darin Adler]

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.

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Committed r184863: <http://trac.webkit.org/changeset/184863>

[Joseph Pecoraro]

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 <[e-mail]>.

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.

[Yusuke Suzuki]

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 <[e-mail]>.
> 
> 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.

[Yusuke Suzuki]

Reopening to attach new patch.

[Yusuke Suzuki]

Created attachment 253725 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 253725 [details]
Patch

thank you for your review ;)

[Dean Jackson]

FWIW, Geoff suggested not defining functions within the prototype implementations. I don't remember the reason unfortunately.

[Yusuke Suzuki]

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

[WebKit Commit Bot]

Comment on attachment 253725 [details]
Patch

Clearing flags on attachment: 253725

Committed r184871: <http://trac.webkit.org/changeset/184871>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.