Add a separate scope for switch

Add a separate scope for switch

The ES2015 specification for switch statements 13.12.11 specifies that
they get their own lexical scope. This patch introduces such a scope
through a complex desugaring in terms of blocks, done so that Crankshaft
does not have to be updated to support multiple constructs providing
scopes.

BUG=v8:4377
LOG=Y
R=adamk

Committed: https://crrev.com/9edbc1f21eb1050cabbe3b8bc9aebf89ada7ebd7
Cr-Commit-Position: refs/heads/master@{#30314}

[Dan Ehrenberg, 2015-08-17 23:37:11]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-17 23:37:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1293283002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1293283002/1

[commit-bot: I haz the power, 2015-08-17 23:46:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-17 23:46:53]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)

[Dan Ehrenberg, 2015-08-18 18:35:47]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-18 18:36:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1293283002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1293283002/20001

[commit-bot: I haz the power, 2015-08-18 18:59:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-18 18:59:18]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2015-08-20 23:33:48]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-20 23:34:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1293283002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1293283002/60001

[commit-bot: I haz the power, 2015-08-20 23:36:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-20 23:36:45]

Dry run: Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/8888)

[Dan Ehrenberg, 2015-08-20 23:52:55]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-20 23:53:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1293283002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1293283002/80001

[Dan Ehrenberg, 2015-08-21 00:05:30]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-21 00:05:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1293283002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1293283002/100001

[commit-bot: I haz the power, 2015-08-21 00:30:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-21 00:30:34]

Dry run: This issue passed the CQ dry run.

[adamk, 2015-08-21 01:19:08]

https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode2982
src/parser.cc:2982: factory()->NewBlock(labels, 2, true,
RelocInfo::kNoPosition);
Seems weird that you pass labels both here and to the switch statement. Are they
both needed? Given that all the blocks end at the same place, it seems like you
should only need the labels (and the target) in one place. Probably on the
switch?

Can you add some tests with labeled breaks inside switch, with labels at various
places (on a block containing a switch, on the switch itself, on a block
contained in the switch)?

https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode2983
src/parser.cc:2983: Scope* switch_scope = NewScope(scope_, BLOCK_SCOPE);
As discussed, I don't think you need this scope (and in fact it'll already be
dropped for you). It's fine to have a block without a scope.

https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode2988
src/parser.cc:2988: // This target exists in case the tag has a do-expression
This comment likely goes away if you simplify the way labels/targets are used
(and it should go away, as do-expressions are not yet a thing)

https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode3010
src/parser.cc:3010: factory()->NewSwitchStatement(labels, switch_pos);
See above, I'd drop the labels here (pass NULL).

https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode3021
src/parser.cc:3021: cases = new (zone()) ZoneList<CaseClause*>(4, zone());
Can you move this initialization up to where cases is declared? Seems like
that'd be simpler.

https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode3034
src/parser.cc:3034: Expect(Token::RBRACE, CHECK_OK);
I think this should move above the scope stuff just above, the closing brace is
effectively the end of the scope.

Can you update the comment about end positions in scopes.h to mention switch
while you're at it?

[Dan Ehrenberg, 2015-08-21 21:50:16]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-21 21:50:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1293283002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1293283002/120001

[commit-bot: I haz the power, 2015-08-21 22:17:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-21 22:17:13]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2015-08-21 23:06:14]

https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode2982
src/parser.cc:2982: factory()->NewBlock(labels, 2, true,
RelocInfo::kNoPosition);
On 2015/08/21 at 01:19:08, adamk wrote:
> Seems weird that you pass labels both here and to the switch statement. Are
they both needed? Given that all the blocks end at the same place, it seems like
you should only need the labels (and the target) in one place. Probably on the
switch?
> 
> Can you add some tests with labeled breaks inside switch, with labels at
various places (on a block containing a switch, on the switch itself, on a block
contained in the switch)?

Done

https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode2983
src/parser.cc:2983: Scope* switch_scope = NewScope(scope_, BLOCK_SCOPE);
On 2015/08/21 at 01:19:08, adamk wrote:
> As discussed, I don't think you need this scope (and in fact it'll already be
dropped for you). It's fine to have a block without a scope.

Removed it.

https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode2988
src/parser.cc:2988: // This target exists in case the tag has a do-expression
On 2015/08/21 at 01:19:08, adamk wrote:
> This comment likely goes away if you simplify the way labels/targets are used
(and it should go away, as do-expressions are not yet a thing)

Done

https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode3010
src/parser.cc:3010: factory()->NewSwitchStatement(labels, switch_pos);
On 2015/08/21 at 01:19:07, adamk wrote:
> See above, I'd drop the labels here (pass NULL).

Passed the labels here, as you suggested above.

https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode3021
src/parser.cc:3021: cases = new (zone()) ZoneList<CaseClause*>(4, zone());
On 2015/08/21 at 01:19:08, adamk wrote:
> Can you move this initialization up to where cases is declared? Seems like
that'd be simpler.

Moved the declaration down to here.

https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode3034
src/parser.cc:3034: Expect(Token::RBRACE, CHECK_OK);
On 2015/08/21 at 01:19:08, adamk wrote:
> I think this should move above the scope stuff just above, the closing brace
is effectively the end of the scope.
> 
> Can you update the comment about end positions in scopes.h to mention switch
while you're at it?

Done

[adamk, 2015-08-21 23:44:00]

lgtm

[Dan Ehrenberg, 2015-08-21 23:52:35]

The CQ bit was checked by littledan@chromium.org

[commit-bot: I haz the power, 2015-08-21 23:52:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1293283002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1293283002/120001

[commit-bot: I haz the power, 2015-08-21 23:54:28]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-08-21 23:54:50]

Patchset 7 (id:??) landed as
https://crrev.com/9edbc1f21eb1050cabbe3b8bc9aebf89ada7ebd7
Cr-Commit-Position: refs/heads/master@{#30314}

[Dan Ehrenberg, 2015-08-22 01:24:02]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1309043004/ by littledan@chromium.org.

The reason for reverting is: Breaks cctest/test-cpu-profiler/SourceLocation on
nosnap.