|
|
Created:
5 years, 8 months ago by conradw Modified:
5 years, 8 months ago Reviewers:
rossberg CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[strong] Implement static restrictions on switch statement
Implements the strong mode proposal's restrictions on the syntax of the
switch statement. Also fixes a minor bug with empty statements in strong
mode and improves StrongUndefinedArrow parser synch tests.
BUG=v8:3956
LOG=N
Committed: https://crrev.com/d8bccfe974a67c6672d4596fa438cb0c83eee8ba
Cr-Commit-Position: refs/heads/master@{#27885}
Patch Set 1 #
Total comments: 10
Patch Set 2 : cl feedback #Patch Set 3 : fix scope issue #
Total comments: 20
Patch Set 4 : cl feedback 2 #Patch Set 5 : minor rename #Patch Set 6 : fixed my broken tests #
Total comments: 15
Patch Set 7 : cl feedback 3 #
Created: 5 years, 8 months ago
Messages
Total messages: 17 (3 generated)
conradw@chromium.org changed reviewers: + rossberg@chromium.org
On 2015/04/14 16:35:59, conradw wrote: > mailto:conradw@chromium.org changed reviewers: > + mailto:rossberg@chromium.org PTAL The error message could probably be better.
Did you forget to git add the JS test by any chance? https://codereview.chromium.org/1084983002/diff/1/src/ast.h File src/ast.h (right): https://codereview.chromium.org/1084983002/diff/1/src/ast.h#newcode283 src/ast.h:283: virtual bool IsStrongSwitchTerminatingStatement() const { return false; } I'm tempted to get rid of this and just use IsJump. AFAICS the only remaining difference is IfStatement. I'm willing to tweak the spec to cover that. Adapting the preparser should be straightforward as well. https://codereview.chromium.org/1084983002/diff/1/src/messages.js File src/messages.js (right): https://codereview.chromium.org/1084983002/diff/1/src/messages.js#newcode172 src/messages.js:172: strong_unterminated_switch: ["In strong mode, each switch statement list must be terminated by break, continue, return or throw"], Nit: strong_switch_fallthrough "In strong mode, switch fall-through is deprecated, terminate each case with 'break', 'continue', 'return' or 'throw'" https://codereview.chromium.org/1084983002/diff/1/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1084983002/diff/1/src/parser.cc#newcode1822 src/parser.cc:1822: if (labels == NULL) { To avoid the code duplication, how about introducing a single helper function, say Statement NewLabelledStatement(ZoneList* labels, Statement) that does the wrapping if lables isn't null, and just returns the statement otherwise. https://codereview.chromium.org/1084983002/diff/1/src/preparser.cc File src/preparser.cc (right): https://codereview.chromium.org/1084983002/diff/1/src/preparser.cc#newcode239 src/preparser.cc:239: if (peek() == Token::SEMICOLON) { Did you add a test covering this? https://codereview.chromium.org/1084983002/diff/1/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1084983002/diff/1/src/preparser.h#newcode1107 src/preparser.h:1107: static PreParserStatement Terminating() { ...and then we can just rename this to Jump as well.
Oops! I did forget to add the JS tests. https://codereview.chromium.org/1084983002/diff/1/src/ast.h File src/ast.h (right): https://codereview.chromium.org/1084983002/diff/1/src/ast.h#newcode283 src/ast.h:283: virtual bool IsStrongSwitchTerminatingStatement() const { return false; } On 2015/04/14 20:19:49, rossberg wrote: > I'm tempted to get rid of this and just use IsJump. AFAICS the only remaining > difference is IfStatement. I'm willing to tweak the spec to cover that. Adapting > the preparser should be straightforward as well. Done. https://codereview.chromium.org/1084983002/diff/1/src/messages.js File src/messages.js (right): https://codereview.chromium.org/1084983002/diff/1/src/messages.js#newcode172 src/messages.js:172: strong_unterminated_switch: ["In strong mode, each switch statement list must be terminated by break, continue, return or throw"], On 2015/04/14 20:19:49, rossberg wrote: > Nit: strong_switch_fallthrough > > "In strong mode, switch fall-through is deprecated, terminate each case with > 'break', 'continue', 'return' or 'throw'" Done. https://codereview.chromium.org/1084983002/diff/1/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1084983002/diff/1/src/parser.cc#newcode1822 src/parser.cc:1822: if (labels == NULL) { On 2015/04/14 20:19:49, rossberg wrote: > To avoid the code duplication, how about introducing a single helper function, > say > > Statement NewLabelledStatement(ZoneList* labels, Statement) > > that does the wrapping if lables isn't null, and just returns the statement > otherwise. Done, with the caveat that the ordering is kind of tricky, I wanted to use this function for the TRY case as well, but it makes assumptions that the wrapper block has already been created before the try-block is parsed. Ended up doing it in two functions. https://codereview.chromium.org/1084983002/diff/1/src/preparser.cc File src/preparser.cc (right): https://codereview.chromium.org/1084983002/diff/1/src/preparser.cc#newcode239 src/preparser.cc:239: if (peek() == Token::SEMICOLON) { On 2015/04/14 20:19:49, rossberg wrote: > Did you add a test covering this? Done, was indirectly covered before but now directly covered. https://codereview.chromium.org/1084983002/diff/1/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1084983002/diff/1/src/preparser.h#newcode1107 src/preparser.h:1107: static PreParserStatement Terminating() { On 2015/04/14 20:19:49, rossberg wrote: > ...and then we can just rename this to Jump as well. Done.
Patchset #3 (id:40001) has been deleted
PTAL, unbroke it
https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc#newcode1886 src/parser.cc:1886: Statement* Parser::ParseWrappedStatement(ZoneList<const AstRawString*>* labels, In fact, I would just inline the switch instead of creating a separate function.. https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc#newcode1905 src/parser.cc:1905: *ok = false; This case should be UNREACHABLE(); https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc#newcode2874 src/parser.cc:2874: !stat->IsJump()) { Nit: stray line break https://codereview.chromium.org/1084983002/diff/60001/src/preparser.cc File src/preparser.cc (right): https://codereview.chromium.org/1084983002/diff/60001/src/preparser.cc#newcod... src/preparser.cc:555: if (statement.IsJumpStatement()) { Nit: Use return with ?: https://codereview.chromium.org/1084983002/diff/60001/src/preparser.cc#newcod... src/preparser.cc:588: stat = (else_stat.IsJumpStatement()) ? stat : Statement::Default(); This doesn't seem quite right, since stat may be something else then either Jump or Default. https://codereview.chromium.org/1084983002/diff/60001/src/preparser.cc#newcod... src/preparser.cc:715: !statement.IsJumpStatement()) { Nit: stray line break But more importantly, don't you need to add "&& token != Token::RBRACE"? https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi... File test/mjsunit/strong/switch.js (right): https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi... test/mjsunit/strong/switch.js:10: let successContexts = [ There are some cases missing, most importantly: switch (1) { default: null; } switch (1) { case 0: case 1: null; } switch (1) { default: {} } Other: blocks, nested blocks, nested if's https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi... test/mjsunit/strong/switch.js:16: ["switch(1) {case 1: if(1) break; else {", "} default: break;};"] Nit: consistent spacing (here and below) https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi... test/mjsunit/strong/switch.js:19: let strongThrowContexts = [ Add some cases here as well, e.g. loops as last statement (even if they end in a jump), illegal cases nested inside a block or if https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi... test/mjsunit/strong/switch.js:63: CheckSwitch("break; "); Nit: Make these an outer loop inside the test function
Where is the changes to Parser? I only see changes to the preparser.
On 2015/04/15 20:55:32, arv wrote: > Where is the changes to Parser? I only see changes to the preparser. https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc#newcode2866 What else are you missing? (Other than that it doesn't allow nested switches yet.)
https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc#newcode1886 src/parser.cc:1886: Statement* Parser::ParseWrappedStatement(ZoneList<const AstRawString*>* labels, On 2015/04/15 19:56:34, rossberg wrote: > In fact, I would just inline the switch instead of creating a separate > function.. Inlining would mean duplicating the code, it has to be called in two places (1829, 1834) due to the stupid Target scoping thing. https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc#newcode1905 src/parser.cc:1905: *ok = false; On 2015/04/15 19:56:34, rossberg wrote: > This case should be UNREACHABLE(); Done. https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc#newcode2874 src/parser.cc:2874: !stat->IsJump()) { On 2015/04/15 19:56:34, rossberg wrote: > Nit: stray line break Done. https://codereview.chromium.org/1084983002/diff/60001/src/preparser.cc File src/preparser.cc (right): https://codereview.chromium.org/1084983002/diff/60001/src/preparser.cc#newcod... src/preparser.cc:555: if (statement.IsJumpStatement()) { On 2015/04/15 19:56:34, rossberg wrote: > Nit: Use return with ?: Done. https://codereview.chromium.org/1084983002/diff/60001/src/preparser.cc#newcod... src/preparser.cc:588: stat = (else_stat.IsJumpStatement()) ? stat : Statement::Default(); On 2015/04/15 19:56:34, rossberg wrote: > This doesn't seem quite right, since stat may be something else then either Jump > or Default. Done. https://codereview.chromium.org/1084983002/diff/60001/src/preparser.cc#newcod... src/preparser.cc:715: !statement.IsJumpStatement()) { On 2015/04/15 19:56:34, rossberg wrote: > Nit: stray line break > > But more importantly, don't you need to add "&& token != Token::RBRACE"? Done, and I made the same mistake in parser https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi... File test/mjsunit/strong/switch.js (right): https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi... test/mjsunit/strong/switch.js:10: let successContexts = [ On 2015/04/15 19:56:34, rossberg wrote: > There are some cases missing, most importantly: > > switch (1) { default: null; } > switch (1) { case 0: case 1: null; } > switch (1) { default: {} } > > Other: blocks, nested blocks, nested if's Done. https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi... test/mjsunit/strong/switch.js:16: ["switch(1) {case 1: if(1) break; else {", "} default: break;};"] On 2015/04/15 19:56:34, rossberg wrote: > Nit: consistent spacing (here and below) Done. https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi... test/mjsunit/strong/switch.js:19: let strongThrowContexts = [ On 2015/04/15 19:56:34, rossberg wrote: > Add some cases here as well, e.g. loops as last statement (even if they end in a > jump), illegal cases nested inside a block or if Done. https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi... test/mjsunit/strong/switch.js:63: CheckSwitch("break; "); On 2015/04/15 19:56:35, rossberg wrote: > Nit: Make these an outer loop inside the test function Done.
Almost there, just a few tweaks to the tests. https://codereview.chromium.org/1084983002/diff/120001/test/cctest/test-parsi... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1084983002/diff/120001/test/cctest/test-parsi... test/cctest/test-parsing.cc:6044: NULL}; Can you add cases with break/continue to label, e.g. by labelling the outer for loop? Also, one example of a nested if (if inside if). https://codereview.chromium.org/1084983002/diff/120001/test/cctest/test-parsi... test/cctest/test-parsing.cc:6055: "case 1: case 2: if (1) break; default:", Also add if (1) break else 0 if (1) 0 else break https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw... File test/mjsunit/strong/switch.js (right): https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw... test/mjsunit/strong/switch.js:9: function CheckSwitch() { I like this much better now. https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw... test/mjsunit/strong/switch.js:12: "continue; ", Add break and continue to label. https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw... test/mjsunit/strong/switch.js:28: "if(1) break; else 1+1; ", Also add the symmetric "if (1) 1+1 else break" https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw... test/mjsunit/strong/switch.js:63: assertDoesNotThrow(strong_wrap[0] + "switch(1) {case 1: " + code + "}}}"); Why not move this case to the successContexts? https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw... test/mjsunit/strong/switch.js:93: assertDoesNotThrow("switch(1) {default: " + code + "}"); Same here? https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw... test/mjsunit/strong/switch.js:102: assertDoesNotThrow("'use strong'; switch(1) {}"); Can't you cover all these by adding "" to the otherStatements?
https://codereview.chromium.org/1084983002/diff/120001/test/cctest/test-parsi... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1084983002/diff/120001/test/cctest/test-parsi... test/cctest/test-parsing.cc:6044: NULL}; On 2015/04/16 12:01:16, rossberg wrote: > Can you add cases with break/continue to label, e.g. by labelling the outer for > loop? > > Also, one example of a nested if (if inside if). Done. https://codereview.chromium.org/1084983002/diff/120001/test/cctest/test-parsi... test/cctest/test-parsing.cc:6055: "case 1: case 2: if (1) break; default:", On 2015/04/16 12:01:16, rossberg wrote: > Also add > > if (1) break else 0 > if (1) 0 else break Done. https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw... File test/mjsunit/strong/switch.js (right): https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw... test/mjsunit/strong/switch.js:12: "continue; ", On 2015/04/16 12:01:16, rossberg wrote: > Add break and continue to label. Done. https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw... test/mjsunit/strong/switch.js:28: "if(1) break; else 1+1; ", On 2015/04/16 12:01:16, rossberg wrote: > Also add the symmetric "if (1) 1+1 else break" Done. https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw... test/mjsunit/strong/switch.js:63: assertDoesNotThrow(strong_wrap[0] + "switch(1) {case 1: " + code + "}}}"); On 2015/04/16 12:01:16, rossberg wrote: > Why not move this case to the successContexts? The successContexts are intended to not work if code is not a jump (so I can have throwing tests inserting otherStatements). If this case was added, line 76 wouldn't come out so neat, since there's no fallthrough from the last case. https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw... test/mjsunit/strong/switch.js:93: assertDoesNotThrow("switch(1) {default: " + code + "}"); On 2015/04/16 12:01:16, rossberg wrote: > Same here? Similar reasoning. This for block is a specific (over)reaction to my mistake with the right bracket condition. https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw... test/mjsunit/strong/switch.js:102: assertDoesNotThrow("'use strong'; switch(1) {}"); On 2015/04/16 12:01:16, rossberg wrote: > Can't you cover all these by adding "" to the otherStatements? It's currently assumed that when one of the otherStatements is used as "code" with a successContext, it will throw in strong mode. Including "" would create empty statements in many cases, causing "allowed fallthroughs", and no throws. The original intention of this section was to test switches with various "especially empty" bodies that didn't fit neatly into one of the lists, but I've ended up with some duplicated tests. Will fix.
lgtm
The CQ bit was checked by conradw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1084983002/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d8bccfe974a67c6672d4596fa438cb0c83eee8ba Cr-Commit-Position: refs/heads/master@{#27885} |