Summary: | [JSC] support Computed Property Names in destructuring Patterns | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Caitlin Potter (:caitp) <caitp> | ||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, ggaren, keith_miller, mark.lam, msaboff, saam | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Caitlin Potter (:caitp)
2015-11-20 08:42:20 PST
Created attachment 265955 [details]
Patch
Support Computed Property names in destructuring patterns, eg
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.
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.
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.
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? > 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
Created attachment 265970 [details]
more tests + changelog + less code duplication
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.
Created attachment 265974 [details]
Patch
Fix email in ChangeLog entry
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.
Comment on attachment 265974 [details]
Patch
Is there anything else needed here?
Comment on attachment 265974 [details] Patch Clearing flags on attachment: 265974 Committed r192768: <http://trac.webkit.org/changeset/192768> All reviewed patches have been landed. Closing bug. |