Bug 142679

Created attachment 254898 [details] patch It begins.

Current implementation probably breaks for function parameters. We should probably change the parser such that function parameters are parsed inside the same parser arena as the function itself.

Created attachment 254906 [details] patch For now, remove support for function parameters and write some tests.

Created attachment 254909 [details] patch

Comment on attachment 254909 [details] patch Forgot to generate test results.

Created attachment 254913 [details] patch With test results.

Comment on attachment 254913 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=254913&action=review > Source/JavaScriptCore/parser/ASTBuilder.h:740 > + node->appendIndex(location, 0, 0); nullptr > Source/JavaScriptCore/parser/Nodes.h:1797 > + Entry(DeconstructionPatternNode* pattern, ExpressionNode* defaultValue) > + : pattern(pattern) > + , defaultValue(defaultValue) > + { > + } No need for this. With C++11 a struct can be initialized with { } syntax anywhere we would use constructor syntax. > Source/JavaScriptCore/parser/Nodes.h:1815 > + m_targetPatterns.append(Entry(identifier, wasString, pattern, defaultValue)); Since this is a struct, I believe you can write: m_targetPatterns.append({ identifier, wasString, pattern, defaultValue }); It’s possible you might have to explicitly specify Entry, but I don’t think so. > Source/JavaScriptCore/parser/Nodes.h:1830 > - Entry(const Identifier& propertyName, bool wasString, DeconstructionPatternNode* pattern) > + Entry(const Identifier& propertyName, bool wasString, DeconstructionPatternNode* pattern, ExpressionNode* defaultValue) > : propertyName(propertyName) > , wasString(wasString) > , pattern(pattern) > + , defaultValue(defaultValue) > { > } No need for this. With C++11 a struct can be initialized with { } syntax anywhere we would use constructor syntax. > Source/JavaScriptCore/parser/Parser.cpp:719 > + TreeExpression defaultValue = 0; > + if (match(EQUAL)) { > + next(TreeBuilder::DontBuildStrings); // consume '=' > + defaultValue = parseAssignmentExpression(context); > + } > + return defaultValue; I think this reads better without a local variable. I would write: if (!match(EQUAL)) return 0; next(TreeBuilder::DontBuildStrings); // consume '=' return parseAssignmentExpression(context);

Comment on attachment 254913 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=254913&action=review >> Source/JavaScriptCore/parser/Nodes.h:1797 >> + } > > No need for this. With C++11 a struct can be initialized with { } syntax anywhere we would use constructor syntax. Is this a matter of policy? I believe that Entry(thing, thing) reads much better than {thing, thing} because we know what we are creating. >> Source/JavaScriptCore/parser/Nodes.h:1815 >> + m_targetPatterns.append(Entry(identifier, wasString, pattern, defaultValue)); > > Since this is a struct, I believe you can write: > > m_targetPatterns.append({ identifier, wasString, pattern, defaultValue }); > > It’s possible you might have to explicitly specify Entry, but I don’t think so. I think it's a lot more clear to see Entry(identifier, wasString, pattern, defaultValue). It's good to know that the struct being created is called Entry. We have a lot of structs in the system, and it's great to know which one we are creating. >> Source/JavaScriptCore/parser/Nodes.h:1830 >> } > > No need for this. With C++11 a struct can be initialized with { } syntax anywhere we would use constructor syntax. Ditto.

I agree with Fil on this one. I like the explicitness of Entry(.)

Created attachment 255091 [details] patch for landing I went with the: "Entry{ . }" syntax which is both explicit and allows us to remove boilerplate constructors. I'll let the CQ land this one for me.

Attachment 255091 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/parser/Nodes.h:1788: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/parser/Nodes.h:1810: Missing space before { [whitespace/braces] [5] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 255091 [details] patch for landing Attachment 255091 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4664861145432064 New failing tests: js/destructuring-assignment-default-values.html

Created attachment 255093 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5

Comment on attachment 255091 [details] patch for landing Attachment 255091 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5612020619542528 New failing tests: js/destructuring-assignment-default-values.html

Created attachment 255095 [details] Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Created attachment 255108 [details] patch for landing Lets try that again.

Attachment 255108 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/parser/Nodes.h:1788: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/parser/Nodes.h:1810: Missing space before { [whitespace/braces] [5] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 255108 [details] patch for landing Clearing flags on attachment: 255108 Committed r185699: <http://trac.webkit.org/changeset/185699>

All reviewed patches have been landed. Closing bug.