Bug 167962 - [ESnext] Implement Object Rest - Implementing Object Rest Destructuring
Summary: [ESnext] Implement Object Rest - Implementing Object Rest Destructuring
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Lima
URL:
Keywords:
Depends on:
Blocks: 167963
  Show dependency treegraph
 
Reported: 2017-02-07 15:37 PST by Caio Lima
Modified: 2017-06-27 20:05 PDT (History)
16 users (show)

See Also:


Attachments
WIP - Patch (12.87 KB, patch)
2017-02-08 18:19 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.02 MB, application/zip)
2017-02-08 19:15 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.67 MB, application/zip)
2017-02-08 19:33 PST, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (1.08 MB, application/zip)
2017-02-08 19:56 PST, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-elcapitan (946.68 KB, application/zip)
2017-02-08 19:59 PST, Build Bot
no flags Details
Patch (37.88 KB, patch)
2017-02-12 16:38 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Benchmark results (83.28 KB, text/plain)
2017-02-17 16:44 PST, Caio Lima
no flags Details
Patch (51.24 KB, patch)
2017-02-25 21:36 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (52.76 KB, patch)
2017-03-04 14:23 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (788.08 KB, application/zip)
2017-03-04 15:48 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (892.52 KB, application/zip)
2017-03-04 15:51 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.73 MB, application/zip)
2017-03-04 15:55 PST, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (880.18 KB, application/zip)
2017-03-04 16:00 PST, Build Bot
no flags Details
Patch (55.00 KB, patch)
2017-03-04 20:36 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (54.98 KB, patch)
2017-03-08 21:29 PST, Caio Lima
no flags Details | Formatted Diff | Diff
WIP - Patch RFC (38.64 KB, patch)
2017-05-15 21:20 PDT, Caio Lima
buildbot: commit-queue-
Details | Formatted Diff | Diff
WIP - Patch RFC (38.60 KB, patch)
2017-05-16 05:33 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (90.92 KB, patch)
2017-05-17 21:29 PDT, Caio Lima
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1005.67 KB, application/zip)
2017-05-18 00:58 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (957.13 KB, application/zip)
2017-05-18 01:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.78 MB, application/zip)
2017-05-18 01:11 PDT, Build Bot
no flags Details
Patch (97.96 KB, patch)
2017-05-18 08:30 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (103.65 KB, patch)
2017-05-18 19:58 PDT, Caio Lima
sbarati: review-
sbarati: commit-queue-
Details | Formatted Diff | Diff
Patch (67.18 KB, patch)
2017-05-21 21:12 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (264.26 KB, patch)
2017-05-27 17:46 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (63.91 KB, patch)
2017-05-27 19:51 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (65.42 KB, patch)
2017-05-31 20:53 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (66.39 KB, patch)
2017-06-03 11:13 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (67.64 KB, patch)
2017-06-13 07:06 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (69.16 KB, patch)
2017-06-20 07:25 PDT, Caio Lima
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.06 MB, application/zip)
2017-06-20 08:51 PDT, Build Bot
no flags Details
Patch (69.14 KB, patch)
2017-06-25 08:08 PDT, Caio Lima
sbarati: review+
Details | Formatted Diff | Diff
Patch (69.42 KB, patch)
2017-06-27 16:55 PDT, Caio Lima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Lima 2017-02-07 15:37:36 PST
Object Rest/Spread Properties for ECMAScript proposal now is in stage 3 and we can implement a prototype of it.

Official proposal: https://github.com/sebmarkbage/ecmascript-rest-spread

This new feature enable the following source code to be parsed and executed:
```
let { x, y, ...z } = { x: 1, y: 2, a: 3, b: 4 };
x; // 1
y; // 2
z; // { a: 3, b: 4 }
```
Comment 1 Caio Lima 2017-02-08 18:19:00 PST
Created attachment 300994 [details]
WIP - Patch

It just starts the implementation. A lot of optimizations need to be done yet and it is not following full specification. I'm going to improve excludedList verification performance using set instead of array.
Comment 2 WebKit Commit Bot 2017-02-08 18:22:15 PST
Attachment 300994 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2143:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2148:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4043:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Build Bot 2017-02-08 19:15:09 PST
Comment on attachment 300994 [details]
WIP - Patch

Attachment 300994 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3029145

New failing tests:
js/parser-syntax-check.html
Comment 4 Build Bot 2017-02-08 19:15:12 PST
Created attachment 300998 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 5 Build Bot 2017-02-08 19:33:55 PST
Comment on attachment 300994 [details]
WIP - Patch

Attachment 300994 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3029332

New failing tests:
js/parser-syntax-check.html
Comment 6 Build Bot 2017-02-08 19:33:58 PST
Created attachment 301001 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 7 Build Bot 2017-02-08 19:56:00 PST
Comment on attachment 300994 [details]
WIP - Patch

Attachment 300994 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3029322

New failing tests:
js/parser-syntax-check.html
Comment 8 Build Bot 2017-02-08 19:56:03 PST
Created attachment 301002 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2017-02-08 19:59:37 PST
Comment on attachment 300994 [details]
WIP - Patch

Attachment 300994 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3029633

New failing tests:
js/parser-syntax-check.html
Comment 10 Build Bot 2017-02-08 19:59:39 PST
Created attachment 301003 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 11 Caio Lima 2017-02-12 16:38:28 PST
Created attachment 301324 [details]
Patch
Comment 12 WebKit Commit Bot 2017-02-12 16:40:00 PST
Attachment 301324 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2143:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2148:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Caio Lima 2017-02-12 16:41:17 PST
(In reply to comment #11)
> Created attachment 301324 [details]
> Patch

This is the proposal for this implementation. I'm thinking in put it behind a flag. Performance comparison is coming soon.
Comment 14 Caio Lima 2017-02-17 16:44:05 PST
Created attachment 302011 [details]
Benchmark results

These changes are neutral.
Comment 15 Keith Miller 2017-02-21 16:22:18 PST
Comment on attachment 301324 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301324&action=review

I think it looks pretty good. I have a few comments, however.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4051
> +            if (UNLIKELY(m_containsRestElement)) {
> +                RefPtr<RegisterID> propertyName = generator.emitLoad(nullptr, target.propertyName);
> +                generator.emitDirectPutByVal(excludedList.get(), excludedIndex.get(), propertyName.get());
> +                generator.emitInc(excludedIndex.get());

Don't we statically know what we are going to put in the excludedList at this point? Can we just add this array to the constant pool? If we do, we should probably freeze so the array can't be modified.

> Source/JavaScriptCore/parser/Parser.cpp:1019
> +                if (kind == DestructuringKind::DestructureToExpressions && !innerPattern)
> +                    return 0;
> +                failIfFalse(innerPattern, "Cannot parse this destructuring pattern");

I think you just want to do propageError(); here

> Source/JavaScriptCore/runtime/JSGlobalObject.h:117
> +    macro(Set, set, set, JSSet, Set, object) \

Why did you move this?
Comment 16 Caio Lima 2017-02-22 18:20:06 PST
Comment on attachment 301324 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301324&action=review

>> Source/JavaScriptCore/parser/Parser.cpp:1019
>> +                failIfFalse(innerPattern, "Cannot parse this destructuring pattern");
> 
> I think you just want to do propageError(); here

Sure. This way we keep the error message more clear.

>> Source/JavaScriptCore/runtime/JSGlobalObject.h:117
>> +    macro(Set, set, set, JSSet, Set, object) \
> 
> Why did you move this?

I'm using it in copyPropertiesData (Source/JavaScriptCore/builtins/GlobalOperations.js) because it makes the algorithm O(n + m) where n = Object.getOwnPropertyNames(source).length and m = excludedList.length. Does it make sense?
Comment 17 Caio Lima 2017-02-22 19:50:02 PST
Comment on attachment 301324 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301324&action=review

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4051
>> +                generator.emitInc(excludedIndex.get());
> 
> Don't we statically know what we are going to put in the excludedList at this point? Can we just add this array to the constant pool? If we do, we should probably freeze so the array can't be modified.

Yes. I agree. However how can we proceed with that? Is it possible to create an JSArray without ExecState? I also have the same question for JSSet. If there is a way to create them here, please let me know. I already know how to get identifiers as constant, but should be good how could I allocate constant Array (better if it is a JSSet).
Comment 18 Caio Lima 2017-02-25 21:36:02 PST
Created attachment 302774 [details]
Patch
Comment 19 WebKit Commit Bot 2017-02-25 21:37:07 PST
Attachment 302774 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2143:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2148:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Caio Lima 2017-02-25 21:37:19 PST
(In reply to comment #18)
> Created attachment 302774 [details]
> Patch

Updated implementation using constant pool to HashSets. Let's see what EWS thinks about it.
Comment 21 Keith Miller 2017-03-03 14:50:03 PST
Comment on attachment 302774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302774&action=review

I think it's looking pretty good, I have a couple of style comments, however.

> Source/JavaScriptCore/builtins/GlobalOperations.js:95
> +        if (!excludedSet.has(nextKey)) {

I think you need to make private symbols for these properties. If a user deletes the has property from Set.prototype your code won't work.

> Source/JavaScriptCore/builtins/GlobalOperations.js:99
> +                @Object.defineProperty(target, nextKey, {value: propValue, enumerable: true, writable: true, configurable: true});

ditto.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1944
> +    if (!setConstantDeconstructionSetRegisters(vm, unlinkedCodeBlock->constantDestructSets()))

I would call this setConstantIdentifierSetRegisters.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:71
> +typedef std::pair<HashSet<UniquedStringImpl*>, unsigned> ConstantDeconstructSetEntry;

I would call this ConstantIndentifierSetEntry

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:230
> +    const Vector<ConstantDeconstructSetEntry>& constantDestructSets() { return m_constantObjDeconstructSets; }

Style: We normally don't abbreviate names like this.

but I would also change this to constantIdentifierSets().

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:461
> +    Vector<ConstantDeconstructSetEntry> m_constantObjDeconstructSets;

ditto.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1927
> +        if (!(entry.first == set))

I would do if (entry.first != set), although you might need to add an operator != to HashSet.

> Source/JavaScriptCore/runtime/ScriptExecutable.cpp:193
> +            // At this point, the CodeBlock creation can fail trying to
> +            // allocate JSSet constants in CodeBlock::setConstantDeconstructionSetRegister.
> +            // Since the only exception from it is OOM, we throw this kind of exception.

I don't think you need this comment.
Comment 22 Caio Lima 2017-03-04 14:23:14 PST
Created attachment 303408 [details]
Patch
Comment 23 WebKit Commit Bot 2017-03-04 14:25:02 PST
Attachment 303408 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2143:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2148:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Caio Lima 2017-03-04 14:25:13 PST
(In reply to comment #22)
> Created attachment 303408 [details]
> Patch

Thank you very much for the review Keith. Updated based in you comments. Let's see what EWS thinks about it.
Comment 25 Build Bot 2017-03-04 15:09:46 PST
Comment on attachment 303408 [details]
Patch

Attachment 303408 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3242966

New failing tests:
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-dfg-eager-no-cjit
stress/object-rest-deconstruct.js.dfg-maximal-flush-validate-no-cjit
stress/object-rest-deconstruct.js.ftl-eager-no-cjit
stress/object-rest-deconstruct.js.ftl-eager
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-no-cjit
stress/object-rest-deconstruct.js.no-ftl
stress/object-rest-deconstruct.js.dfg-eager-no-cjit-validate
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-ftl
stress/object-rest-deconstruct.js.ftl-no-cjit-small-pool
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-eager-no-cjit
stress/object-rest-deconstruct.js.ftl-no-cjit-no-inline-validate
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-llint
stress/object-rest-deconstruct.js.ftl-no-cjit-validate-sampling-profiler
stress/object-rest-deconstruct.js.ftl-no-cjit-no-put-stack-validate
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout
stress/object-rest-deconstruct.js.default
stress/object-rest-deconstruct.js.no-cjit-collect-continuously
stress/object-rest-deconstruct.js.no-cjit-validate-phases
stress/object-rest-deconstruct.js.no-llint
stress/object-rest-deconstruct.js.dfg-eager
Comment 26 Build Bot 2017-03-04 15:48:04 PST
Comment on attachment 303408 [details]
Patch

Attachment 303408 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3243039

New failing tests:
js/parser-syntax-check.html
Comment 27 Build Bot 2017-03-04 15:48:11 PST
Created attachment 303411 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 28 Build Bot 2017-03-04 15:51:36 PST
Comment on attachment 303408 [details]
Patch

Attachment 303408 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3243050

New failing tests:
js/parser-syntax-check.html
Comment 29 Build Bot 2017-03-04 15:51:42 PST
Created attachment 303412 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 30 Build Bot 2017-03-04 15:55:05 PST
Comment on attachment 303408 [details]
Patch

Attachment 303408 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3243052

New failing tests:
js/parser-syntax-check.html
Comment 31 Build Bot 2017-03-04 15:55:11 PST
Created attachment 303413 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 32 Build Bot 2017-03-04 16:00:22 PST
Comment on attachment 303408 [details]
Patch

Attachment 303408 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3243053

New failing tests:
js/parser-syntax-check.html
Comment 33 Build Bot 2017-03-04 16:00:26 PST
Created attachment 303415 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 34 Caio Lima 2017-03-04 20:36:48 PST
Created attachment 303438 [details]
Patch
Comment 35 WebKit Commit Bot 2017-03-04 20:39:49 PST
Attachment 303438 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2143:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2148:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 36 Caio Lima 2017-03-07 19:32:29 PST
Ping review
Comment 37 Keith Miller 2017-03-08 09:29:49 PST
Comment on attachment 303438 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303438&action=review

One more, small set of changes then the patch should be gtg. Thanks for following through on this!

> Source/JavaScriptCore/builtins/GlobalOperations.js:94
> +    for (let nextKey of keys) {

I don't think this can be a for of loop since it uses the iterator protocol, which is, sadly, observable. Instead you should do an indexed loop. Getting the length property of keys should be fine since it's an array.

For some reason I thought I made this comment last time but I guess I forgot :(.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2410
> +    for (size_t i = 0; i < count; i++) {

I think you could do:
for (const auto& entry : constants)

Your current code will use the copy constructor of HashSet, which we can avoid.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2418
> +        HashSet<UniquedStringImpl*> set = entry.first;

you can make this:
 const HashSet<UniquedStringImpl*>& set = entry.first; 

for the same reason as above.
Comment 38 Caio Lima 2017-03-08 21:25:24 PST
Comment on attachment 303438 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303438&action=review

Thank you very much for the reviews

>> Source/JavaScriptCore/builtins/GlobalOperations.js:94
>> +    for (let nextKey of keys) {
> 
> I don't think this can be a for of loop since it uses the iterator protocol, which is, sadly, observable. Instead you should do an indexed loop. Getting the length property of keys should be fine since it's an array.
> 
> For some reason I thought I made this comment last time but I guess I forgot :(.

You are right. Thanks catching it. Changed.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2410
>> +    for (size_t i = 0; i < count; i++) {
> 
> I think you could do:
> for (const auto& entry : constants)
> 
> Your current code will use the copy constructor of HashSet, which we can avoid.

Agreed and changed.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2418
>> +        HashSet<UniquedStringImpl*> set = entry.first;
> 
> you can make this:
>  const HashSet<UniquedStringImpl*>& set = entry.first; 
> 
> for the same reason as above.

Ditto.
Comment 39 Caio Lima 2017-03-08 21:29:36 PST
Created attachment 303896 [details]
Patch
Comment 40 WebKit Commit Bot 2017-03-08 21:31:26 PST
Attachment 303896 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2143:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2148:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 41 Keith Miller 2017-03-09 10:15:43 PST
Comment on attachment 303896 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303896&action=review

r=me.

> Source/JavaScriptCore/builtins/GlobalOperations.js:92
> +    let from = @toObject(source);

Nit: You could just do:
@Object(source)
here but this is probably fine.
Comment 42 Caio Lima 2017-03-09 15:20:54 PST
Comment on attachment 303896 [details]
Patch

Thank you very much for the review Keith!
Comment 43 WebKit Commit Bot 2017-03-09 18:00:37 PST
Comment on attachment 303896 [details]
Patch

Clearing flags on attachment: 303896

Committed r213697: <http://trac.webkit.org/changeset/213697>
Comment 44 WebKit Commit Bot 2017-03-09 18:00:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 45 Saam Barati 2017-05-11 18:26:33 PDT
Comment on attachment 303896 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303896&action=review

> Source/JavaScriptCore/parser/Parser.cpp:1017
> +                if (kind == DestructuringKind::DestructureToExpressions && !innerPattern)

This line of code looks wrong. Why do we only care about DestructureToExpressions?
Comment 46 Caio Lima 2017-05-12 09:22:56 PDT
Comment on attachment 303896 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303896&action=review

>> Source/JavaScriptCore/parser/Parser.cpp:1017
>> +                if (kind == DestructuringKind::DestructureToExpressions && !innerPattern)
> 
> This line of code looks wrong. Why do we only care about DestructureToExpressions?

I think you are right. We should care about other cases as well IMHO. I'm going to handle that in another bug.

Also I don't remember any test to check the condition when innerPatter != null. We are missing this piece of code.
Comment 47 Mark Lam 2017-05-15 17:02:16 PDT
I'm going to roll this out.  Let's re-land after we have a proper fix all the issues.
Comment 48 Mark Lam 2017-05-15 17:23:56 PDT
This has been rolled out in r216891: <http://trac.webkit.org/r216891>.
Comment 49 Caio Lima 2017-05-15 21:20:35 PDT
Created attachment 310219 [details]
WIP - Patch RFC

The problem of last implementation is that it was considering that excludedList was always statically defined, but the example below shows that it's not true:

```
let a = "foo";
let {[a]: b, ...r} = {foo: 2, bar 3};
// b == 2;
// r.bar == 3;
```

This way, I decided change my approach and emit bytecode to fill excludedList, and then now we can carry the case above properly.
I added the test was in our suite but I would like to check EWS and get feedback as well here.

The previous implementations was faster and I'm thinking in a way to allow static usage that probably will the common case for this feature, but I think it's worth it bring it in another Patch. 

Also, I think it's a good idea put this feature behind a flag before commit.
Comment 50 Build Bot 2017-05-15 21:22:47 PDT
Attachment 310219 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 51 Build Bot 2017-05-15 22:07:30 PDT
Comment on attachment 310219 [details]
WIP - Patch RFC

Attachment 310219 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3748187

New failing tests:
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout
stress/object-rest-deconstruct.js.dfg-eager-no-cjit-validate
stress/object-rest-deconstruct.js.no-ftl
stress/object-rest-deconstruct.js.no-llint
stress/object-rest-deconstruct.js.ftl-eager-no-cjit-b3o1
stress/object-rest-deconstruct.js.ftl-eager
stress/object-rest-deconstruct.js.ftl-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-ftl
stress/object-rest-deconstruct.js.ftl-no-cjit-small-pool
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-eager-no-cjit
stress/object-rest-deconstruct.js.ftl-no-cjit-no-inline-validate
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-dfg-eager-no-cjit
stress/object-rest-deconstruct.js.default
stress/object-rest-deconstruct.js.dfg-maximal-flush-validate-no-cjit
stress/object-rest-deconstruct.js.dfg-eager
stress/object-rest-deconstruct.js.ftl-eager-no-cjit
stress/object-rest-deconstruct.js.ftl-no-cjit-validate-sampling-profiler
stress/object-rest-deconstruct.js.ftl-no-cjit-no-put-stack-validate
stress/object-rest-deconstruct.js.no-cjit-collect-continuously
stress/object-rest-deconstruct.js.no-cjit-validate-phases
Comment 52 Caio Lima 2017-05-16 05:33:09 PDT
Created attachment 310256 [details]
WIP - Patch RFC

Fixing failures in EWS.
Comment 53 Build Bot 2017-05-16 05:34:06 PDT
Attachment 310256 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 54 Mark Lam 2017-05-16 10:25:24 PDT
Comment on attachment 310256 [details]
WIP - Patch RFC

View in context: https://bugs.webkit.org/attachment.cgi?id=310256&action=review

> Source/JavaScriptCore/parser/Nodes.h:2133
> -            m_targetPatterns.append(Entry { Identifier(), propertyExpression, false, pattern, defaultValue });
> +            m_targetPatterns.append(Entry{ Identifier(), propertyExpression, false, pattern, defaultValue, bindingType });

Don't pass a stack instance of Identifier here.  Instead, you should have appendEntry() take a VM&, and pass vm.propertyNames->nullIdentifier here instead.
Comment 55 Saam Barati 2017-05-16 11:05:05 PDT
(In reply to Caio Lima from comment #49)
> Created attachment 310219 [details]
> WIP - Patch RFC
> 
> The problem of last implementation is that it was considering that
> excludedList was always statically defined, but the example below shows that
> it's not true:
> 
> ```
> let a = "foo";
> let {[a]: b, ...r} = {foo: 2, bar 3};
> // b == 2;
> // r.bar == 3;
> ```
> 
> This way, I decided change my approach and emit bytecode to fill
> excludedList, and then now we can carry the case above properly.
> I added the test was in our suite but I would like to check EWS and get
> feedback as well here.
This sounds right to me when we have computed properties. However, I think it's a good optimization to build the list at compile time when we know all the properties statically. So I think we should have a combination of the old code and the new code depending on some simple static analysis.

> 
> The previous implementations was faster and I'm thinking in a way to allow
> static usage that probably will the common case for this feature, but I
> think it's worth it bring it in another Patch. 
> 
> Also, I think it's a good idea put this feature behind a flag before commit.
Yeah this sounds good. You could probably just put the feature check in the parser.
Comment 56 Caio Lima 2017-05-17 21:29:18 PDT
Created attachment 310484 [details]
Patch

Fixed Mark's and Saam's comments. I also created the build flags. Let's see EWS results and it's also ready for another review round.
Comment 57 Build Bot 2017-05-17 21:30:56 PDT
Attachment 310484 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/mac/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebCore/PAL/Configurations/FeatureDefines.xcconfig). Use sync-feature-defines if possible.  [featuredefines/equality] [5]
ERROR: Source/WebCore/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebCore/PAL/Configurations/FeatureDefines.xcconfig). Use sync-feature-defines if possible.  [featuredefines/equality] [5]
ERROR: Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebCore/PAL/Configurations/FeatureDefines.xcconfig). Use sync-feature-defines if possible.  [featuredefines/equality] [5]
ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebCore/PAL/Configurations/FeatureDefines.xcconfig). Use sync-feature-defines if possible.  [featuredefines/equality] [5]
ERROR: Source/JavaScriptCore/parser/ASTBuilder.h:953:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/WebKit2/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebCore/PAL/Configurations/FeatureDefines.xcconfig). Use sync-feature-defines if possible.  [featuredefines/equality] [5]
ERROR: Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4101:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 9 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 58 Build Bot 2017-05-17 22:18:20 PDT
Comment on attachment 310484 [details]
Patch

Attachment 310484 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3766894

New failing tests:
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout
stress/object-rest-deconstruct.js.dfg-eager-no-cjit-validate
stress/object-rest-deconstruct.js.no-ftl
stress/object-rest-deconstruct.js.no-llint
stress/object-rest-deconstruct.js.ftl-eager-no-cjit-b3o1
stress/object-rest-deconstruct.js.ftl-eager
stress/object-rest-deconstruct.js.ftl-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-ftl
stress/object-rest-deconstruct.js.ftl-no-cjit-small-pool
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-eager-no-cjit
stress/object-rest-deconstruct.js.ftl-no-cjit-no-inline-validate
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-dfg-eager-no-cjit
stress/object-rest-deconstruct.js.default
stress/object-rest-deconstruct.js.dfg-maximal-flush-validate-no-cjit
stress/object-rest-deconstruct.js.dfg-eager
stress/object-rest-deconstruct.js.ftl-eager-no-cjit
stress/object-rest-deconstruct.js.ftl-no-cjit-validate-sampling-profiler
stress/object-rest-deconstruct.js.ftl-no-cjit-no-put-stack-validate
stress/object-rest-deconstruct.js.no-cjit-collect-continuously
stress/object-rest-deconstruct.js.no-cjit-validate-phases
Comment 59 Build Bot 2017-05-18 00:58:50 PDT
Comment on attachment 310484 [details]
Patch

Attachment 310484 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3767437

New failing tests:
js/parser-syntax-check.html
Comment 60 Build Bot 2017-05-18 00:58:52 PDT
Created attachment 310495 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 61 Build Bot 2017-05-18 01:01:47 PDT
Comment on attachment 310484 [details]
Patch

Attachment 310484 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3767439

New failing tests:
js/parser-syntax-check.html
Comment 62 Build Bot 2017-05-18 01:01:48 PDT
Created attachment 310496 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 63 Build Bot 2017-05-18 01:10:59 PDT
Comment on attachment 310484 [details]
Patch

Attachment 310484 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3767443

New failing tests:
js/parser-syntax-check.html
Comment 64 Build Bot 2017-05-18 01:11:01 PDT
Created attachment 310498 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 65 Caio Lima 2017-05-18 08:30:16 PDT
Created attachment 310510 [details]
Patch

Fixed flag configuration.
Comment 66 Build Bot 2017-05-18 08:32:41 PDT
Attachment 310510 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/ASTBuilder.h:953:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4101:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 4 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 67 Caio Lima 2017-05-18 19:58:40 PDT
Created attachment 310597 [details]
Patch

This version is disabling the recursive destructuring and in this implementation the following code results in syntax error:

```
let {a, ...{b}} = {a: 1, b: 2};
```
Comment 68 Build Bot 2017-05-18 20:01:53 PDT
Attachment 310597 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 69 Saam Barati 2017-05-21 14:32:05 PDT
Comment on attachment 310597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310597&action=review

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:78
> +ENABLE_ESNEXT_OBJ_REST_SPREAD = ENABLE_ESNEXT_OBJ_REST_SPREAD;

Lets make it a runtime option, not a compile time one.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:890
> +        m_constantRegisters[entry.second].set(vm, this, JSValue(jsSet));

I don't think there is a need for the JSValue(jsSet) here.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1967
> +    unsigned index = m_nextConstantOffset;
> +    m_constantPoolRegisters.append(FirstConstantRegisterIndex + m_nextConstantOffset);
> +    ++m_nextConstantOffset;
> +    m_codeBlock->addSetConstant(set);
> +    RegisterID* m_setRegister = &m_constantPoolRegisters[index];

It looks like we do this work in a few places, perhaps we can write a function that abstracts it, like:
```
unsigned addConstantIndex()
{
    unsigned index = m_nextConstantOffset;
    m_constantPoolRegisters.append(FirstConstantRegisterIndex + m_nextConstantOffset);
    ++m_nextConstantOffset;
    return index;
}
or even make the function return std::pair<unsigned, RegisterID*>
```
and then you can just be like:
``` unsigned index = addConstantIndex();```

Maybe it's not worth it, but just a suggestion since I'm not a big fan of repeating nearly identical code.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4071
> +        excludedList = generator.emitNewArray(generator.newTemporary(), 0, 0);
> +        excludedIndex = generator.newTemporary();
> +        generator.emitLoad(excludedIndex.get(), jsNumber(0));

Please don't do this. You should just build the Set in bytecode. There is no need to first build an array, and then have your builtin convert the array to a set.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4079
> +                // Should not emit get_by_id for indexed ones.

No need for this comment. We have code like this all over the place.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:-4076
> -            RefPtr<RegisterID> propertyName = generator.emitNode(target.propertyExpression);

Can you add an assertion here of what the bindingType is? Also, I think it's worth adding an assertion that this is the last thing in the list, otherwise this algorithm is broken.

> Source/JavaScriptCore/parser/Parser.cpp:980
> +        if (kind == DestructuringKind::DestructureToExpressions)
> +        return 0;

bad indentation

> Source/JavaScriptCore/parser/Parser.cpp:1070
> +            if (UNLIKELY(match(DOTDOTDOT))) {

Is this syntactically required to be the last element in the destrucruting clause?
Also, no need for UNLIKELY, we're just going to negatively impact parsing code that uses this.
Comment 70 Caio Lima 2017-05-21 15:50:46 PDT
Comment on attachment 310597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310597&action=review

>> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:78
>> +ENABLE_ESNEXT_OBJ_REST_SPREAD = ENABLE_ESNEXT_OBJ_REST_SPREAD;
> 
> Lets make it a runtime option, not a compile time one.

ok

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4071
>> +        generator.emitLoad(excludedIndex.get(), jsNumber(0));
> 
> Please don't do this. You should just build the Set in bytecode. There is no need to first build an array, and then have your builtin convert the array to a set.

I also think it's better use JSSet, but there is a thing that I'm worried:

If our implementation uses JSSet directly and then user overrides the Set.prototype.add, the rest semantics will be broken.

like:

```
Set.prototype.add = () => false;

let {a, b, ...r} = {a: 1, b: 2, c: 3, d: 4};

// here "r" will contain "a" and "b", and it will be wrong.

```

Do you think it is right?

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:-4076
>> -            RefPtr<RegisterID> propertyName = generator.emitNode(target.propertyExpression);
> 
> Can you add an assertion here of what the bindingType is? Also, I think it's worth adding an assertion that this is the last thing in the list, otherwise this algorithm is broken.

ok

>> Source/JavaScriptCore/parser/Parser.cpp:980
>> +        return 0;
> 
> bad indentation

Oops.

>> Source/JavaScriptCore/parser/Parser.cpp:1070
>> +            if (UNLIKELY(match(DOTDOTDOT))) {
> 
> Is this syntactically required to be the last element in the destrucruting clause?
> Also, no need for UNLIKELY, we're just going to negatively impact parsing code that uses this.

Yes. I will remove the UNLIKELY here.
Comment 71 Saam Barati 2017-05-21 16:31:53 PDT
(In reply to Caio Lima from comment #70)
> Comment on attachment 310597 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310597&action=review
> 
> >> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:78
> >> +ENABLE_ESNEXT_OBJ_REST_SPREAD = ENABLE_ESNEXT_OBJ_REST_SPREAD;
> > 
> > Lets make it a runtime option, not a compile time one.
> 
> ok
> 
> >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4071
> >> +        generator.emitLoad(excludedIndex.get(), jsNumber(0));
> > 
> > Please don't do this. You should just build the Set in bytecode. There is no need to first build an array, and then have your builtin convert the array to a set.
> 
> I also think it's better use JSSet, but there is a thing that I'm worried:
> 
> If our implementation uses JSSet directly and then user overrides the
> Set.prototype.add, the rest semantics will be broken.
> 
This is an issue in all our builtins. They take care yo use private identifiers. You’ll need to do the same here, and make sure SetPrototye sets up your private identifier to the function you want. 
> like:
> 
> ```
> Set.prototype.add = () => false;
> 
> let {a, b, ...r} = {a: 1, b: 2, c: 3, d: 4};
> 
> // here "r" will contain "a" and "b", and it will be wrong.
> 
> ```
> 
> Do you think it is right?
> 
> >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:-4076
> >> -            RefPtr<RegisterID> propertyName = generator.emitNode(target.propertyExpression);
> > 
> > Can you add an assertion here of what the bindingType is? Also, I think it's worth adding an assertion that this is the last thing in the list, otherwise this algorithm is broken.
> 
> ok
> 
> >> Source/JavaScriptCore/parser/Parser.cpp:980
> >> +        return 0;
> > 
> > bad indentation
> 
> Oops.
> 
> >> Source/JavaScriptCore/parser/Parser.cpp:1070
> >> +            if (UNLIKELY(match(DOTDOTDOT))) {
> > 
> > Is this syntactically required to be the last element in the destrucruting clause?
> > Also, no need for UNLIKELY, we're just going to negatively impact parsing code that uses this.
> 
> Yes. I will remove the UNLIKELY here.
Comment 72 Saam Barati 2017-05-21 16:32:25 PDT
You also need to set up an identifier to retrieve the Set constructor from global scope.
Comment 73 Caio Lima 2017-05-21 16:53:53 PDT
(In reply to Saam Barati from comment #72)
> You also need to set up an identifier to retrieve the Set constructor from
> global scope.
You are right. I didn't update it before because I was thinking it wasn't possible, but I just realized that ```copyDataProperties``` is called this way. I'm going to fix it ASAP.
Comment 74 Caio Lima 2017-05-21 21:12:49 PDT
Created attachment 310831 [details]
Patch

Addressed Saam's review
Comment 75 Build Bot 2017-05-21 21:14:59 PDT
Attachment 310831 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 76 Yusuke Suzuki 2017-05-27 11:47:49 PDT
Comment on attachment 310831 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310831&action=review

I haven't read all the code yet, quick comments.

> Source/JavaScriptCore/builtins/GlobalOperations.js:99
> +            let desc = @Object.@getOwnPropertyDescriptor(from, nextKey);
> +            if (desc.enumerable && desc !== @undefined) {

You should use `@propertyIsEnumerable(from, nextKey)` instead (See Object.assign implementation). This is more efficient.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:71
> +typedef std::pair<IdentifierSet, unsigned> ConstantIndentifierSetEntry;

For newly added code, we should use `using ConstantIndentifierSetEntry = std::pair<IdentifierSet, unsigned>;` instead.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:107
> +    JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->builtinNames().getOwnPropertyDescriptorPrivateName(), objectConstructorGetOwnPropertyDescriptor, DontEnum, 1);

It is not necessary.

> Source/JavaScriptCore/runtime/SetPrototype.cpp:71
> +    JSC_NATIVE_INTRINSIC_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->builtinNames().addPrivateName(), setProtoFuncAdd, DontEnum, 1, JSSetHasIntrinsic);

This is not correct. Set#add should not have JSSetHasIntrinsic.

> Source/WTF/wtf/FeatureDefines.h:64
> +

This is not necessary.

> Source/cmake/WebKitFeatures.cmake:111
> +    WEBKIT_OPTION_DEFINE(ENABLE_ESNEXT_OBJ_REST_SPREAD "Toggle ESNext Object Rest/Spread support" PRIVATE ON)

This is not necessary.
Comment 77 Caio Lima 2017-05-27 17:12:02 PDT
Comment on attachment 310831 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310831&action=review

>> Source/JavaScriptCore/builtins/GlobalOperations.js:99
>> +            if (desc.enumerable && desc !== @undefined) {
> 
> You should use `@propertyIsEnumerable(from, nextKey)` instead (See Object.assign implementation). This is more efficient.

Thanks for that!

>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:71
>> +typedef std::pair<IdentifierSet, unsigned> ConstantIndentifierSetEntry;
> 
> For newly added code, we should use `using ConstantIndentifierSetEntry = std::pair<IdentifierSet, unsigned>;` instead.

Done.

>> Source/JavaScriptCore/runtime/SetPrototype.cpp:71
>> +    JSC_NATIVE_INTRINSIC_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->builtinNames().addPrivateName(), setProtoFuncAdd, DontEnum, 1, JSSetHasIntrinsic);
> 
> This is not correct. Set#add should not have JSSetHasIntrinsic.

oops. Fixed.

>> Source/WTF/wtf/FeatureDefines.h:64
>> +
> 
> This is not necessary.

Removed.
Comment 78 Caio Lima 2017-05-27 17:12:49 PDT
(In reply to Yusuke Suzuki from comment #76)
> Comment on attachment 310831 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310831&action=review
> 
Thanks for the Review!
Comment 79 Caio Lima 2017-05-27 17:46:58 PDT
Created attachment 311428 [details]
Patch

Addressing Yusuki's comments.
Comment 80 Caio Lima 2017-05-27 19:51:21 PDT
Created attachment 311431 [details]
Patch

Rebased with upstream code.
Comment 81 Build Bot 2017-05-27 19:53:50 PDT
Attachment 311431 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 82 Saam Barati 2017-05-31 14:22:19 PDT
Comment on attachment 311431 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311431&action=review

> JSTests/ChangeLog:3
> +        [ESnext] Implement Object Rest - Implementing Object Rest Destructuring

Does this patch do spread?

> Source/JavaScriptCore/ChangeLog:13
> +        array identifiers of already destructed properties, following spec draft

your implementation uses a Set, not an array

> Source/JavaScriptCore/builtins/GlobalOperations.js:100
> +                @Object.@defineProperty(target, nextKey, {value: propValue, enumerable: true, writable: true, configurable: true});

Don't we have a fast version of this?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:887
> +            jsSet->add(exec, JSValue(jsString));

style nit: I don't think there is a need for the JSValue(jsString) here, I think you can just do jsString.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4103
> +                    RefPtr<RegisterID> addMethod = generator.emitGetById(generator.newTemporary(), excludedList.get(), generator.propertyNames().builtinNames().addPrivateName());
> +                    CallArguments args(generator, nullptr, 1);
> +                    generator.emitMove(args.thisRegister(), excludedList.get());
> +                    generator.emitMove(args.argumentRegister(0), propertyName.get());
> +                    generator.emitCall(generator.newTemporary(), addMethod.get(), NoExpectedFunction, args, divot(), divotStart(), divotEnd(), DebuggableCall::No);

It's unfortunate to repeat this getById on every property. I think you can hoist this.
Also, it's interesting we're relying on on Set.prototype being non-writable and non-configurable. Can you add a test perhaps that just asserts this is true (if we don't already have one).
Perhaps such a test that would fail if it's not the case.

> Source/JavaScriptCore/parser/Parser.cpp:914
> +    auto element = parseMemberExpression(context);

why is this a member expression? Member expression allows "new"

> Source/JavaScriptCore/parser/Parser.cpp:1069
> +            if (Options::useObjectRestSpread() && match(DOTDOTDOT)) {

Nit: I think there is a chance it's more performant to swap the lhs and rhs of this &&.

> Source/JavaScriptCore/parser/Parser.cpp:3859
> +        if (Options::useObjectRestSpread())

the worlds tiniest nit suggestion: maybe it's worth caching this option in a parser field and perhaps it'll be hotter in cache.
Comment 83 Caio Lima 2017-05-31 16:13:19 PDT
Comment on attachment 311431 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311431&action=review

>> JSTests/ChangeLog:3
>> +        [ESnext] Implement Object Rest - Implementing Object Rest Destructuring
> 
> Does this patch do spread?

No. Spread is another Patch that will come after this one.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4103
>> +                    generator.emitCall(generator.newTemporary(), addMethod.get(), NoExpectedFunction, args, divot(), divotStart(), divotEnd(), DebuggableCall::No);
> 
> It's unfortunate to repeat this getById on every property. I think you can hoist this.
> Also, it's interesting we're relying on on Set.prototype being non-writable and non-configurable. Can you add a test perhaps that just asserts this is true (if we don't already have one).
> Perhaps such a test that would fail if it's not the case.

Yes, you are right here. It's totally not efficient.

I can test if Set.prototype is non-configurable or non-writable, but by spec, it should be: https://tc39.github.io/ecma262/#sec-set.prototype.

>> Source/JavaScriptCore/parser/Parser.cpp:914
>> +    auto element = parseMemberExpression(context);
> 
> why is this a member expression? Member expression allows "new"

I actually just removed the recursive approach, i.e, not allowing "{" nor "[". I'm not sure if the prop spec changed, but it's unclear to me what should we allow here. Do you have any suggestion?

>> Source/JavaScriptCore/parser/Parser.cpp:1069
>> +            if (Options::useObjectRestSpread() && match(DOTDOTDOT)) {
> 
> Nit: I think there is a chance it's more performant to swap the lhs and rhs of this &&.

Changed.

>> Source/JavaScriptCore/parser/Parser.cpp:3859
>> +        if (Options::useObjectRestSpread())
> 
> the worlds tiniest nit suggestion: maybe it's worth caching this option in a parser field and perhaps it'll be hotter in cache.

I think it's a good idea.
Comment 84 Caio Lima 2017-05-31 16:13:30 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=311431&action=review

>> JSTests/ChangeLog:3
>> +        [ESnext] Implement Object Rest - Implementing Object Rest Destructuring
> 
> Does this patch do spread?

No. Spread is another Patch that will come after this one.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4103
>> +                    generator.emitCall(generator.newTemporary(), addMethod.get(), NoExpectedFunction, args, divot(), divotStart(), divotEnd(), DebuggableCall::No);
> 
> It's unfortunate to repeat this getById on every property. I think you can hoist this.
> Also, it's interesting we're relying on on Set.prototype being non-writable and non-configurable. Can you add a test perhaps that just asserts this is true (if we don't already have one).
> Perhaps such a test that would fail if it's not the case.

Yes, you are right here. It's totally not efficient.

I can test if Set.prototype is non-configurable or non-writable, but by spec, it should be: https://tc39.github.io/ecma262/#sec-set.prototype.

>> Source/JavaScriptCore/parser/Parser.cpp:914
>> +    auto element = parseMemberExpression(context);
> 
> why is this a member expression? Member expression allows "new"

I actually just removed the recursive approach, i.e, not allowing "{" nor "[". I'm not sure if the prop spec changed, but it's unclear to me what should we allow here. Do you have any suggestion?

>> Source/JavaScriptCore/parser/Parser.cpp:1069
>> +            if (Options::useObjectRestSpread() && match(DOTDOTDOT)) {
> 
> Nit: I think there is a chance it's more performant to swap the lhs and rhs of this &&.

Changed.

>> Source/JavaScriptCore/parser/Parser.cpp:3859
>> +        if (Options::useObjectRestSpread())
> 
> the worlds tiniest nit suggestion: maybe it's worth caching this option in a parser field and perhaps it'll be hotter in cache.

I think it's a good idea.
Comment 85 Caio Lima 2017-05-31 16:13:42 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=311431&action=review

>> JSTests/ChangeLog:3
>> +        [ESnext] Implement Object Rest - Implementing Object Rest Destructuring
> 
> Does this patch do spread?

No. Spread is another Patch that will come after this one.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4103
>> +                    generator.emitCall(generator.newTemporary(), addMethod.get(), NoExpectedFunction, args, divot(), divotStart(), divotEnd(), DebuggableCall::No);
> 
> It's unfortunate to repeat this getById on every property. I think you can hoist this.
> Also, it's interesting we're relying on on Set.prototype being non-writable and non-configurable. Can you add a test perhaps that just asserts this is true (if we don't already have one).
> Perhaps such a test that would fail if it's not the case.

Yes, you are right here. It's totally not efficient.

I can test if Set.prototype is non-configurable or non-writable, but by spec, it should be: https://tc39.github.io/ecma262/#sec-set.prototype.

>> Source/JavaScriptCore/parser/Parser.cpp:914
>> +    auto element = parseMemberExpression(context);
> 
> why is this a member expression? Member expression allows "new"

I actually just removed the recursive approach, i.e, not allowing "{" nor "[". I'm not sure if the prop spec changed, but it's unclear to me what should we allow here. Do you have any suggestion?

>> Source/JavaScriptCore/parser/Parser.cpp:1069
>> +            if (Options::useObjectRestSpread() && match(DOTDOTDOT)) {
> 
> Nit: I think there is a chance it's more performant to swap the lhs and rhs of this &&.

Changed.

>> Source/JavaScriptCore/parser/Parser.cpp:3859
>> +        if (Options::useObjectRestSpread())
> 
> the worlds tiniest nit suggestion: maybe it's worth caching this option in a parser field and perhaps it'll be hotter in cache.

I think it's a good idea.
Comment 86 Caio Lima 2017-05-31 16:13:56 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=311431&action=review

Thank you very much for the review!

>> JSTests/ChangeLog:3
>> +        [ESnext] Implement Object Rest - Implementing Object Rest Destructuring
> 
> Does this patch do spread?

No. Spread is another Patch that will come after this one.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4103
>> +                    generator.emitCall(generator.newTemporary(), addMethod.get(), NoExpectedFunction, args, divot(), divotStart(), divotEnd(), DebuggableCall::No);
> 
> It's unfortunate to repeat this getById on every property. I think you can hoist this.
> Also, it's interesting we're relying on on Set.prototype being non-writable and non-configurable. Can you add a test perhaps that just asserts this is true (if we don't already have one).
> Perhaps such a test that would fail if it's not the case.

Yes, you are right here. It's totally not efficient.

I can test if Set.prototype is non-configurable or non-writable, but by spec, it should be: https://tc39.github.io/ecma262/#sec-set.prototype.

>> Source/JavaScriptCore/parser/Parser.cpp:914
>> +    auto element = parseMemberExpression(context);
> 
> why is this a member expression? Member expression allows "new"

I actually just removed the recursive approach, i.e, not allowing "{" nor "[". I'm not sure if the prop spec changed, but it's unclear to me what should we allow here. Do you have any suggestion?

>> Source/JavaScriptCore/parser/Parser.cpp:1069
>> +            if (Options::useObjectRestSpread() && match(DOTDOTDOT)) {
> 
> Nit: I think there is a chance it's more performant to swap the lhs and rhs of this &&.

Changed.

>> Source/JavaScriptCore/parser/Parser.cpp:3859
>> +        if (Options::useObjectRestSpread())
> 
> the worlds tiniest nit suggestion: maybe it's worth caching this option in a parser field and perhaps it'll be hotter in cache.

I think it's a good idea.
Comment 87 Caio Lima 2017-05-31 20:53:14 PDT
Created attachment 311679 [details]
Patch

Changed Patch based in Saam's review
Comment 88 Build Bot 2017-05-31 20:56:32 PDT
Attachment 311679 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 89 Caio Lima 2017-06-03 11:13:34 PDT
Created attachment 311933 [details]
Patch
Comment 90 Build Bot 2017-06-03 11:15:30 PDT
Attachment 311933 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 91 Caio Lima 2017-06-03 11:21:09 PDT
(In reply to Caio Lima from comment #89)
> Created attachment 311933 [details]
> Patch

About memberExpression usage in "parseObjectRestAssignmentElement", it isn't allowing "new" expressions because we test "element && context.isAssignmentLocation(element)" just after memberExpressions parsing and fail if it's false.
Comment 92 Caio Lima 2017-06-09 06:00:52 PDT
Ping review
Comment 93 Saam Barati 2017-06-12 12:28:10 PDT
Comment on attachment 311933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311933&action=review

Patch looks good to me overall. I have one main suggestion for your builtin, and also some tests we should add before landing.

> Source/JavaScriptCore/builtins/GlobalOperations.js:100
> +                @Object.@defineProperty(target, nextKey, {value: propValue, enumerable: true, writable: true, configurable: true});

I thought we added a bytecode to define property using an integer bit set. If so, we should use that here, so we don't allocate objects on every loop iteration.
Yusuke, does this bytecode exist?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:881
> +        JSSet* jsSet = JSSet::create(exec, vm, setStructure);

Nit: It might make sense to add a constructor to JSSet indicating how many items it'll have in it. This can be done in a follow up. That way, we don't end up doing any reallocation of its backing store when adding items to it.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4062
> +

Nice. This function looks good to me

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4089
> +                    RefPtr<RegisterID> pIndex = generator.emitLoad(nullptr, jsNumber(optionalIndex.value()));

Style nit: We typically don't names webkit. I'd name this either "index" or "propertyIndex"

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4135
> +            generator.emitMove(args.argumentRegister(argumentCount++), newObject.get());
> +            generator.emitMove(args.argumentRegister(argumentCount++), rhs);
> +            if (m_containsComputedProperty)
> +                generator.emitMove(args.argumentRegister(argumentCount++), excludedList.get());
> +            else {
> +                RefPtr<RegisterID> excludedSetReg = generator.emitLoad(generator.newTemporary(), excludedSet);
> +                generator.emitMove(args.argumentRegister(argumentCount++), excludedSetReg.get());
> +            }

Why not just 0, 1, 2 for these as constants instead of using the argumentCount variable?

> Source/JavaScriptCore/parser/Parser.cpp:922
> +    if (strictMode() && m_parserState.lastIdentifier && context.isResolve(element)) {
> +        bool isEvalOrArguments = m_vm->propertyNames->eval == *m_parserState.lastIdentifier || m_vm->propertyNames->arguments == *m_parserState.lastIdentifier;
> +        failIfTrueIfStrict(isEvalOrArguments, "Cannot modify '", m_parserState.lastIdentifier->impl(), "' in strict mode");
> +    }

Please add a test for this. Also, please add tests for object rest being used in "catch", along with the expected syntax errors

> Source/JavaScriptCore/parser/Parser.cpp:981
> +        if (kind == DestructuringKind::DestructureToExpressions)
> +            return 0;

This branch is unreachable. I'd assert at the top of this function that this isn't the case.

> Source/JavaScriptCore/parser/Parser.cpp:986
> +    failIfTrue(match(LET) && (kind == DestructuringKind::DestructureToLet || kind == DestructuringKind::DestructureToConst), "Cannot use 'let' as an identifier name for a LexicalDeclaration");
> +    semanticFailIfTrue(isDisallowedIdentifierAwait(m_token), "Cannot use 'await' as a ", destructuringKindToVariableKindName(kind), " ", disallowedIdentifierAwaitReason());

Please add tests for these if you haven't already.

> Source/JavaScriptCore/parser/Parser.cpp:989
> +    m_parserState.nonLHSCount = nonLHSCount;

What's the point of this? It doesn't look like this gets changed.

> Source/JavaScriptCore/parser/Parser.cpp:998
> +    return parseObjectRestElement(context, kind, exportType, duplicateIdentifier, bindingContext);

Are we expected to get a syntax error here:
``` catch({...foo.bar})```? If so, please add a test to ensure we have this behavior.
Comment 94 Caio Lima 2017-06-12 22:53:35 PDT
Comment on attachment 311933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311933&action=review

Thank you very much for the review. I've made the changes and I'm now running JSC tests before send the Patch.

>> Source/JavaScriptCore/builtins/GlobalOperations.js:100
>> +                @Object.@defineProperty(target, nextKey, {value: propValue, enumerable: true, writable: true, configurable: true});
> 
> I thought we added a bytecode to define property using an integer bit set. If so, we should use that here, so we don't allocate objects on every loop iteration.
> Yusuke, does this bytecode exist?

IRC, I will work in the fast path here in https://bugs.webkit.org/show_bug.cgi?id=172888. But should be good Yusuke confirms that anyways =)

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:881
>> +        JSSet* jsSet = JSSet::create(exec, vm, setStructure);
> 
> Nit: It might make sense to add a constructor to JSSet indicating how many items it'll have in it. This can be done in a follow up. That way, we don't end up doing any reallocation of its backing store when adding items to it.

I agree. Created bug for that https://bugs.webkit.org/show_bug.cgi?id=173297

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4089
>> +                    RefPtr<RegisterID> pIndex = generator.emitLoad(nullptr, jsNumber(optionalIndex.value()));
> 
> Style nit: We typically don't names webkit. I'd name this either "index" or "propertyIndex"

Fixed

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4135
>> +            }
> 
> Why not just 0, 1, 2 for these as constants instead of using the argumentCount variable?

I was just following previous code pattern. Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:922
>> +    }
> 
> Please add a test for this. Also, please add tests for object rest being used in "catch", along with the expected syntax errors

Done.

>> Source/JavaScriptCore/parser/Parser.cpp:986
>> +    semanticFailIfTrue(isDisallowedIdentifierAwait(m_token), "Cannot use 'await' as a ", destructuringKindToVariableKindName(kind), " ", disallowedIdentifierAwaitReason());
> 
> Please add tests for these if you haven't already.

Done.

>> Source/JavaScriptCore/parser/Parser.cpp:989
>> +    m_parserState.nonLHSCount = nonLHSCount;
> 
> What's the point of this? It doesn't look like this gets changed.

I think you are right. Removed.

>> Source/JavaScriptCore/parser/Parser.cpp:998
>> +    return parseObjectRestElement(context, kind, exportType, duplicateIdentifier, bindingContext);
> 
> Are we expected to get a syntax error here:
> ``` catch({...foo.bar})```? If so, please add a test to ensure we have this behavior.

No. added the test.
Comment 95 Caio Lima 2017-06-13 07:06:15 PDT
Created attachment 312769 [details]
Patch

Fixed Nits and added tests following Saam's review
Comment 96 Build Bot 2017-06-13 07:08:46 PDT
Attachment 312769 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 97 Yusuke Suzuki 2017-06-17 07:16:02 PDT
Comment on attachment 311933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311933&action=review

>>> Source/JavaScriptCore/builtins/GlobalOperations.js:100
>>> +                @Object.@defineProperty(target, nextKey, {value: propValue, enumerable: true, writable: true, configurable: true});
>> 
>> I thought we added a bytecode to define property using an integer bit set. If so, we should use that here, so we don't allocate objects on every loop iteration.
>> Yusuke, does this bytecode exist?
> 
> IRC, I will work in the fast path here in https://bugs.webkit.org/show_bug.cgi?id=172888. But should be good Yusuke confirms that anyways =)

You can use op_define_data_property bytecode operation here. It can avoids costly object allocation for property descriptors. (Currently, Object.defineProperty is implemented in C++. Thus we cannot avoid this object allocation by using Object Allocation Sinking.
https://bugs.webkit.org/show_bug.cgi?id=172888 will solve it. But this can be leveraged only in FTL.

Thus, if we can use op_define_data_property, using it is the best way.
You can emit it by introducing some bytecode intrinsic.
Comment 98 Caio Lima 2017-06-20 07:25:16 PDT
Created attachment 313398 [details]
Patch

Using intrinsic byte code to create data property.
Comment 99 Build Bot 2017-06-20 07:28:09 PDT
Attachment 313398 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2148:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2153:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 100 Build Bot 2017-06-20 08:51:06 PDT
Comment on attachment 313398 [details]
Patch

Attachment 313398 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3966284

New failing tests:
webrtc/video-replace-muted-track.html
Comment 101 Build Bot 2017-06-20 08:51:07 PDT
Created attachment 313400 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 102 Caio Lima 2017-06-25 08:08:59 PDT
Created attachment 313802 [details]
Patch

I'm adding intrinsic use in defineProperty to avoid Descriptor Object allocation.
Comment 103 Build Bot 2017-06-25 08:11:32 PDT
Attachment 313802 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2148:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2153:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 104 Saam Barati 2017-06-27 12:37:55 PDT
Comment on attachment 313802 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313802&action=review

r=me

> JSTests/stress/object-rest-deconstruct.js:204
> +

Please add tests for object rest in catch.
Comment 105 Caio Lima 2017-06-27 16:55:10 PDT
Created attachment 313959 [details]
Patch

Updating tests and ChangeLog
Comment 106 Build Bot 2017-06-27 17:01:22 PDT
Attachment 313959 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2147:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2152:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 107 WebKit Commit Bot 2017-06-27 20:05:50 PDT
Comment on attachment 313959 [details]
Patch

Clearing flags on attachment: 313959

Committed r218861: <http://trac.webkit.org/changeset/218861>
Comment 108 WebKit Commit Bot 2017-06-27 20:05:53 PDT
All reviewed patches have been landed.  Closing bug.