Bug 145107 - [ES6] Implement Array.prototype.copyWithin
Summary: [ES6] Implement Array.prototype.copyWithin
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on: 144128
Blocks:
  Show dependency treegraph
 
Reported: 2015-05-17 10:58 PDT by Yusuke Suzuki
Modified: 2019-03-20 18:09 PDT (History)
11 users (show)

See Also:


Attachments
Patch (18.96 KB, patch)
2015-05-24 10:02 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-mavericks (567.13 KB, application/zip)
2015-05-24 10:52 PDT, Build Bot
no flags Details
Patch (21.72 KB, patch)
2015-05-24 10:55 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (663.62 KB, application/zip)
2015-05-24 11:12 PDT, Build Bot
no flags Details
Patch (23.68 KB, patch)
2015-05-24 11:16 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (23.66 KB, patch)
2015-05-26 10:34 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (6.53 KB, patch)
2015-05-26 13:17 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-05-17 10:58:05 PDT
[ES6] Implement Array.prototype.copyWithin
Comment 1 Yusuke Suzuki 2015-05-24 10:02:53 PDT
Created attachment 253657 [details]
Patch
Comment 2 Build Bot 2015-05-24 10:52:00 PDT
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
Comment 3 Build Bot 2015-05-24 10:52:04 PDT
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
Comment 4 Yusuke Suzuki 2015-05-24 10:55:14 PDT
Created attachment 253660 [details]
Patch
Comment 5 Build Bot 2015-05-24 11:12:51 PDT
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
Comment 6 Build Bot 2015-05-24 11:12:56 PDT
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
Comment 7 Yusuke Suzuki 2015-05-24 11:16:46 PDT
Created attachment 253662 [details]
Patch
Comment 8 Yusuke Suzuki 2015-05-25 05:14:55 PDT
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 9 Darin Adler 2015-05-26 09:29:31 PDT
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 10 Yusuke Suzuki 2015-05-26 10:32:35 PDT
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.
Comment 11 Yusuke Suzuki 2015-05-26 10:34:53 PDT
Created attachment 253708 [details]
Patch
Comment 12 Darin Adler 2015-05-26 10:54:42 PDT
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 13 Yusuke Suzuki 2015-05-26 11:00:54 PDT
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
Comment 14 Yusuke Suzuki 2015-05-26 11:24:17 PDT
Committed r184863: <http://trac.webkit.org/changeset/184863>
Comment 15 Joseph Pecoraro 2015-05-26 11:58:09 PDT
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 16 Yusuke Suzuki 2015-05-26 12:27:09 PDT
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.
Comment 17 Yusuke Suzuki 2015-05-26 13:17:06 PDT
Reopening to attach new patch.
Comment 18 Yusuke Suzuki 2015-05-26 13:17:10 PDT
Created attachment 253725 [details]
Patch
Comment 19 Yusuke Suzuki 2015-05-26 13:27:30 PDT
Comment on attachment 253725 [details]
Patch

thank you for your review ;)
Comment 20 Dean Jackson 2015-05-26 14:03:23 PDT
FWIW, Geoff suggested not defining functions within the prototype implementations. I don't remember the reason unfortunately.
Comment 21 Yusuke Suzuki 2015-05-26 14:10:32 PDT
(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 22 WebKit Commit Bot 2015-05-26 14:17:39 PDT
Comment on attachment 253725 [details]
Patch

Clearing flags on attachment: 253725

Committed r184871: <http://trac.webkit.org/changeset/184871>
Comment 23 WebKit Commit Bot 2015-05-26 14:17:43 PDT
All reviewed patches have been landed.  Closing bug.