[JSC] support Computed Property Names in destructuring Patterns

[Caitlin Potter (:caitp)]

[JSC] support Computed Property Names in destructuring Patterns

[Caitlin Potter (:caitp)]

Created attachment 265955 [details]
Patch

Support Computed Property names in destructuring patterns, eg

[WebKit Commit Bot]

Attachment 265955 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2020:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2025:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caitlin Potter (:caitp)]

Created attachment 265956 [details]
Patch

fixed changelog style --- check-webkit-style doesn't seem to approve of the style that was in the tree to begin with, I dunno.

[WebKit Commit Bot]

Attachment 265956 [details] did not pass style-queue:


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


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 265956 [details]
Patch

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

r=me with some comments

> Source/JavaScriptCore/ChangeLog:8
> +        Add support for computed property names in destructuring BindingPatterns

It's worth adding a link to the spec here.

> Source/JavaScriptCore/parser/Parser.cpp:891
> +                    consumeOrFail(COLON, "Expected ':' after property name");

So this kind of expression is only allowed using the new feature you added recently that assigns to the rhs of the colon?

> Source/JavaScriptCore/parser/Parser.cpp:894
> +                    TreeExpression defaultValue = parseDefaultValueForDestructuringPattern(context);

Can we add a test for this case?

> Source/JavaScriptCore/tests/es6/destructuring_assignment_computed_properties.js:1
> +function test() {

I think this test is great. But, it's a bit tricky to follow. Can you also add just a couple really straight forward tests?

[Caitlin Potter (:caitp)]

> So this kind of expression is only allowed using the new feature you added recently that assigns to the rhs of the colon?

It was already possible to assign to the rhs of the colon, the consumeOrFail and other code duplication is here to keep it a bit more straight forward (instead of conditionally deciding how to behave depending on whether propertyExpression is null or not, further down). I don't have a strong opinion on it if people don't like the code duplication in the name of straightforwardness.

---

Adding some other tests

[Caitlin Potter (:caitp)]

Created attachment 265970 [details]
more tests + changelog + less code duplication

[WebKit Commit Bot]

Attachment 265970 [details] did not pass style-queue:


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


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caitlin Potter (:caitp)]

Created attachment 265974 [details]
Patch

Fix email in ChangeLog entry

[WebKit Commit Bot]

Attachment 265974 [details] did not pass style-queue:


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


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caitlin Potter (:caitp)]

Comment on attachment 265974 [details]
Patch

Is there anything else needed here?

[WebKit Commit Bot]

Comment on attachment 265974 [details]
Patch

Clearing flags on attachment: 265974

Committed r192768: <http://trac.webkit.org/changeset/192768>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.