Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1107053002:
Parsing binding patterns. (Closed)
|
Created:
5 years, 8 months ago by Dmitry Lomov (no reviews) Modified:
5 years, 7 months ago CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionParsing binding patterns.
Just parsing, no desugaring yet.
R=arv@chromium.org,rossberg@chromium.org
BUG=v8:811
LOG=N
Committed: https://crrev.com/80bf5686fa78cacf49200c6a15b249aa61ded25e
Cr-Commit-Position: refs/heads/master@{#28112}
Patch Set 1 #
Total comments: 17
Patch Set 2 : CR feedback #Patch Set 3 : Minor fix to formatter flags #
Total comments: 5
Patch Set 4 : CR feedback #
Total comments: 3
Messages
Total messages: 24 (8 generated)
PTAL - I think I covered all cases for binding patterns.
LGTM modulo tests https://codereview.chromium.org/1107053002/diff/1/src/preparser.cc File src/preparser.cc (right): https://codereview.chromium.org/1107053002/diff/1/src/preparser.cc#newcode514 src/preparser.cc:514: // Parse variable name. Nit: update this comment https://codereview.chromium.org/1107053002/diff/1/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1107053002/diff/1/src/preparser.h#newcode689 src/preparser.h:689: void BindingPatternUnexpectedToken(ExpressionClassifier* classifier) { Nit: is this function worth it, especially since it only applies in some of the cases? https://codereview.chromium.org/1107053002/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1107053002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:6360: const char* data[] = {"a", "{ x : y }", "[a]", "[a,b,c]", "{ x : x, y : y }", Add [], {}, and {x}, and a couple of nested cases like [{x, y: [{}]}]
Not sure how complete you want this to be? https://codereview.chromium.org/1107053002/diff/1/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1107053002/diff/1/src/preparser.h#newcode689 src/preparser.h:689: void BindingPatternUnexpectedToken(ExpressionClassifier* classifier) { On 2015/04/27 17:20:24, rossberg wrote: > Nit: is this function worth it, especially since it only applies in some of the > cases? I think it is worth it. The code gets much more readable with it. https://codereview.chromium.org/1107053002/diff/1/src/preparser.h#newcode3024 src/preparser.h:3024: classifier->RecordBindingPatternError( Maybe introduce a RecordPatternError that does both? https://codereview.chromium.org/1107053002/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1107053002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:6360: const char* data[] = {"a", "{ x : y }", "[a]", "[a,b,c]", "{ x : x, y : y }", Nit: Don't let `git cl format` bully you. These things are way easier to read if there is one test per line. https://codereview.chromium.org/1107053002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:6360: const char* data[] = {"a", "{ x : y }", "[a]", "[a,b,c]", "{ x : x, y : y }", How about? - nested patterns - array elision - rest - intializers - property shorthand https://codereview.chromium.org/1107053002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:6378: "!a", "{ x : y++ }", "[a++]", "(x => y)", "a[i]", "a()", "a.b", "new a", also "() => x" https://codereview.chromium.org/1107053002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:6381: "null", "true", "false", "1", "'abc'", "class {}", NULL}; How about some keywords, eval/arguments and strict future keywords (and yield in generator/outside generator)
PTAL https://codereview.chromium.org/1107053002/diff/1/src/preparser.cc File src/preparser.cc (right): https://codereview.chromium.org/1107053002/diff/1/src/preparser.cc#newcode514 src/preparser.cc:514: // Parse variable name. On 2015/04/27 17:20:24, rossberg wrote: > Nit: update this comment Done. https://codereview.chromium.org/1107053002/diff/1/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1107053002/diff/1/src/preparser.h#newcode689 src/preparser.h:689: void BindingPatternUnexpectedToken(ExpressionClassifier* classifier) { On 2015/04/27 17:56:55, arv wrote: > On 2015/04/27 17:20:24, rossberg wrote: > > Nit: is this function worth it, especially since it only applies in some of > the > > cases? > > I think it is worth it. The code gets much more readable with it. +1 to arv. Added more usages. https://codereview.chromium.org/1107053002/diff/1/src/preparser.h#newcode3024 src/preparser.h:3024: classifier->RecordBindingPatternError( On 2015/04/27 17:56:55, arv wrote: > Maybe introduce a RecordPatternError that does both? On second thoughts, I removed all RecordAssignmentPatternErrors for now, since I do not have tests for them and they are likely incorrect. https://codereview.chromium.org/1107053002/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1107053002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:6360: const char* data[] = {"a", "{ x : y }", "[a]", "[a,b,c]", "{ x : x, y : y }", On 2015/04/27 17:56:55, arv wrote: > How about? > > - nested patterns > - array elision > - rest > - intializers > - property shorthand > > Added everything I support yet. Rest, initializers and property shorthands are not yet supported. https://codereview.chromium.org/1107053002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:6360: const char* data[] = {"a", "{ x : y }", "[a]", "[a,b,c]", "{ x : x, y : y }", On 2015/04/27 17:56:55, arv wrote: > Nit: Don't let `git cl format` bully you. These things are way easier to read if > there is one test per line. No! All hail 'git cl format'! Our Blessed Format-Leader is always right. Done. https://codereview.chromium.org/1107053002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:6360: const char* data[] = {"a", "{ x : y }", "[a]", "[a,b,c]", "{ x : x, y : y }", On 2015/04/27 17:20:24, rossberg wrote: > Add [], {}, and {x}, and a couple of nested cases like [{x, y: [{}]}] Done. https://codereview.chromium.org/1107053002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:6378: "!a", "{ x : y++ }", "[a++]", "(x => y)", "a[i]", "a()", "a.b", "new a", On 2015/04/27 17:56:55, arv wrote: > also "() => x" Done. https://codereview.chromium.org/1107053002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:6381: "null", "true", "false", "1", "'abc'", "class {}", NULL}; On 2015/04/27 17:56:55, arv wrote: > How about some keywords, eval/arguments and strict future keywords (and yield in > generator/outside generator) Done.
The CQ bit was checked by dslomov@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from rossberg@chromium.org Link to the patchset: https://codereview.chromium.org/1107053002/#ps20001 (title: "CR feedback")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1107053002/20001
The CQ bit was checked by dslomov@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from rossberg@chromium.org Link to the patchset: https://codereview.chromium.org/1107053002/#ps40001 (title: "Minor fix to formatter flags")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1107053002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/1107053002/diff/40001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1107053002/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:6369: "[{x:x, y:y}, [a,b,c]]", moar "{var: x}", "{42: x}", "{+2: x}", "{-2: x}", "{'hi': x}, https://codereview.chromium.org/1107053002/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:6423: "false", moar // keywords "var", // nested "[var]", "{x: {y: var}}",
The CQ bit was checked by dslomov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org, rossberg@chromium.org Link to the patchset: https://codereview.chromium.org/1107053002/#ps60001 (title: "CR feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1107053002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/80bf5686fa78cacf49200c6a15b249aa61ded25e Cr-Commit-Position: refs/heads/master@{#28112}
Message was sent while issue was closed.
https://codereview.chromium.org/1107053002/diff/40001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1107053002/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:6369: "[{x:x, y:y}, [a,b,c]]", On 2015/04/28 13:42:14, arv wrote: > moar > > "{var: x}", > "{42: x}", > "{+2: x}", > "{-2: x}", > "{'hi': x}, Done. {+2: x} and {-2: x} are not valid AFAIU. https://codereview.chromium.org/1107053002/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:6423: "false", On 2015/04/28 13:42:14, arv wrote: > moar > > // keywords > "var", > > // nested > "[var]", > "{x: {y: var}}", Done.
Message was sent while issue was closed.
https://codereview.chromium.org/1107053002/diff/40001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1107053002/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:6369: "[{x:x, y:y}, [a,b,c]]", On 2015/04/28 16:01:36, Dmitry Lomov (chromium) wrote: > On 2015/04/28 13:42:14, arv wrote: > > moar > > > > "{var: x}", > > "{42: x}", > > "{+2: x}", > > "{-2: x}", > > "{'hi': x}, > > Done. {+2: x} and {-2: x} are not valid AFAIU. You are right... but 42e-2 is... not needed though. https://codereview.chromium.org/1107053002/diff/60001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1107053002/diff/60001/test/cctest/test-parsin... test/cctest/test-parsing.cc:6372: "{'hi' : x}", I still want `{var: x}` to make sure we are not treating the property name as a binding.
Message was sent while issue was closed.
On 2015/04/28 16:07:23, arv wrote: > https://codereview.chromium.org/1107053002/diff/40001/test/cctest/test-parsin... > File test/cctest/test-parsing.cc (right): > > https://codereview.chromium.org/1107053002/diff/40001/test/cctest/test-parsin... > test/cctest/test-parsing.cc:6369: "[{x:x, y:y}, [a,b,c]]", > On 2015/04/28 16:01:36, Dmitry Lomov (chromium) wrote: > > On 2015/04/28 13:42:14, arv wrote: > > > moar > > > > > > "{var: x}", > > > "{42: x}", > > > "{+2: x}", > > > "{-2: x}", > > > "{'hi': x}, > > > > Done. {+2: x} and {-2: x} are not valid AFAIU. > > You are right... but 42e-2 is... not needed though. > > https://codereview.chromium.org/1107053002/diff/60001/test/cctest/test-parsin... > File test/cctest/test-parsing.cc (right): > > https://codereview.chromium.org/1107053002/diff/60001/test/cctest/test-parsin... > test/cctest/test-parsing.cc:6372: "{'hi' : x}", > I still want `{var: x}` to make sure we are not treating the property name as a > binding. Done in https://codereview.chromium.org/1112073002/
Message was sent while issue was closed.
wingo@igalia.com changed reviewers: + wingo@igalia.com
Message was sent while issue was closed.
drive-by question/nit https://codereview.chromium.org/1107053002/diff/60001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1107053002/diff/60001/src/preparser.h#newcode... src/preparser.h:3592: } A bit late, but: wouldn't this be more appropriate in ParseAssignmentExpression, where we detect the => in the first place? ParseArrowFunctionLiteral is also called from other places where the classifier isn't necessary, e.g. when lazily parsing when the pre-parser already proved that it was an arrow function.
Message was sent while issue was closed.
https://codereview.chromium.org/1107053002/diff/60001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1107053002/diff/60001/src/preparser.h#newcode691 src/preparser.h:691: scanner()->location(), "unexpected_token", Token::String(peek())); peek_location(), I think |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
