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 }
```
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.
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.
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
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
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
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
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.
(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 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 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 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).
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.
(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 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.
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.
(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.
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
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
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
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
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 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 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.
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 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.
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.
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.
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 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.
(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.
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.
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.
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
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
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
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.
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};
```
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 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 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.
(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.
(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.
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 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 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.
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 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 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.
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.
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.
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.
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.
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.
(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 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 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.
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 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.
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.
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
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.
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.
2017-02-08 18:19 PST, Caio Lima
2017-02-08 19:15 PST, Build Bot
2017-02-08 19:33 PST, Build Bot
2017-02-08 19:56 PST, Build Bot
2017-02-08 19:59 PST, Build Bot
2017-02-12 16:38 PST, Caio Lima
2017-02-17 16:44 PST, Caio Lima
2017-02-25 21:36 PST, Caio Lima
2017-03-04 14:23 PST, Caio Lima
2017-03-04 15:48 PST, Build Bot
2017-03-04 15:51 PST, Build Bot
2017-03-04 15:55 PST, Build Bot
2017-03-04 16:00 PST, Build Bot
2017-03-04 20:36 PST, Caio Lima
2017-03-08 21:29 PST, Caio Lima
2017-05-15 21:20 PDT, Caio Lima
2017-05-16 05:33 PDT, Caio Lima
2017-05-17 21:29 PDT, Caio Lima
2017-05-18 00:58 PDT, Build Bot
2017-05-18 01:01 PDT, Build Bot
2017-05-18 01:11 PDT, Build Bot
2017-05-18 08:30 PDT, Caio Lima
2017-05-18 19:58 PDT, Caio Lima
saam: commit-queue-
2017-05-21 21:12 PDT, Caio Lima
2017-05-27 17:46 PDT, Caio Lima
2017-05-27 19:51 PDT, Caio Lima
2017-05-31 20:53 PDT, Caio Lima
2017-06-03 11:13 PDT, Caio Lima
2017-06-13 07:06 PDT, Caio Lima
2017-06-20 07:25 PDT, Caio Lima
2017-06-20 08:51 PDT, Build Bot
2017-06-25 08:08 PDT, Caio Lima
2017-06-27 16:55 PDT, Caio Lima