Implement switch statement, without the "complex switch statement" (aka switch statement with conti…

Implement switch statement, without the "complex switch statement" (aka switch statement with continue to a label).

Breaking this change up into two changes, as there's one last bug I'm tracking down on switch with continue.

BUG=https://github.com/dart-lang/sdk/issues/27394
R=sra@google.com

Committed: https://github.com/dart-lang/sdk/commit/67445d409e2017b5d4592ac03154bd9e0803ff85

[Emily Fortuna, 2017-01-14 00:06:40]

Patchset #1 (id:1) has been deleted

[Emily Fortuna, 2017-01-14 00:13:14]

efortuna@google.com changed reviewers:
+ het@google.com, sra@google.com

[Emily Fortuna, 2017-01-14 00:13:14]

I ended up splitting this up into two CLs, but you may see some things in this
CL that hint at more to come... 

This change puts us at 82.1%. More soon!!

[sra1, 2017-01-14 03:18:23]

Initial comments

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:1041: /// [isLoopJump] is [:true:]
when the jump handler is for a loop. This is used
[:x:] -->  `x`

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:1155: if ((new
ContinueVisitor(switchCase.body)).containsContinue) {
Maybe make the visitor have a static method and private constructor

   if (ContinueVisitor.hasContinue(switchCase.body)) ...

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:1176: bool
_isDefaultCase(ir.SwitchCase switchCase) =>
could be static

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/graph_builder.dart (right):

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/graph_builder.dart:39: /// important for unexpected
control flow analysis (returns, breaks, etc
I would not call it 'unexpected'.
Perhaps say 'Used when generating control flow out of a try block'

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart (right):

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart:44: final Map<ir.TreeNode,
KernelJumpTarget> _jumpTargets =
Are they still ir.Statements?

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart:936: ir.Node targetStatement;
ir.Statement ?

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart:955: new LabelDefinitionX(null,
'l${index++}', this)..setBreakTarget());
Figure out how to make the label consistent.
Maybe use 'L' instead of 'l'.

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/switch_handler.dart (right):

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/switch_handler.dart:6: class ContinueVisitor extends
ir.Visitor {
Perhaps call is something more specific, e.g. SwitchContinueAnalysis
Name the file the 'same'.

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/switch_handler.dart:12: }
If there are new statements, will this do the right thing, or should the default
be overridden to throw?

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/switch_handler.dart:15: containsContinue = true;
Do we want to know if it targets a specific switch (think of nested switch
statements, which one has the continue?)

[Emily Fortuna, 2017-01-17 23:33:09]

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:1041: /// [isLoopJump] is [:true:]
when the jump handler is for a loop. This is used
On 2017/01/14 03:18:22, sra1 wrote:
> [:x:] -->  `x`

Done.

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:1155: if ((new
ContinueVisitor(switchCase.body)).containsContinue) {
On 2017/01/14 03:18:22, sra1 wrote:
> Maybe make the visitor have a static method and private constructor
> 
>    if (ContinueVisitor.hasContinue(switchCase.body)) ...

Done.

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:1176: bool
_isDefaultCase(ir.SwitchCase switchCase) =>
On 2017/01/14 03:18:22, sra1 wrote:
> could be static

Done.

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/graph_builder.dart (right):

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/graph_builder.dart:39: /// important for unexpected
control flow analysis (returns, breaks, etc
On 2017/01/14 03:18:22, sra1 wrote:
> I would not call it 'unexpected'.
> Perhaps say 'Used when generating control flow out of a try block'

Good point. Updated.

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart (right):

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart:44: final Map<ir.TreeNode,
KernelJumpTarget> _jumpTargets =
On 2017/01/14 03:18:22, sra1 wrote:
> Are they still ir.Statements?

See below.

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart:936: ir.Node targetStatement;
On 2017/01/14 03:18:22, sra1 wrote:
> ir.Statement ?

Unfortunately no. This can be a SwitchCase, which is not a statement.

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart:955: new LabelDefinitionX(null,
'l${index++}', this)..setBreakTarget());
On 2017/01/14 03:18:22, sra1 wrote:
> Figure out how to make the label consistent.
> Maybe use 'L' instead of 'l'.

what do you mean by consistent? I made the capitalization change.

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/switch_handler.dart (right):

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/switch_handler.dart:6: class ContinueVisitor extends
ir.Visitor {
On 2017/01/14 03:18:22, sra1 wrote:
> Perhaps call is something more specific, e.g. SwitchContinueAnalysis
> Name the file the 'same'.

Done.

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/switch_handler.dart:12: }
On 2017/01/14 03:18:23, sra1 wrote:
> If there are new statements, will this do the right thing, or should the
default
> be overridden to throw?

Done.

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/switch_handler.dart:15: containsContinue = true;
On 2017/01/14 03:18:23, sra1 wrote:
> Do we want to know if it targets a specific switch (think of nested switch
> statements, which one has the continue?)

added TODO

[sra1, 2017-01-18 23:31:29]

lgtm

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart (right):

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart:936: ir.Node targetStatement;
On 2017/01/17 23:33:09, Emily Fortuna wrote:
> On 2017/01/14 03:18:22, sra1 wrote:
> > ir.Statement ?
> 
> Unfortunately no. This can be a SwitchCase, which is not a statement.

Ok, perhaps say this in the comment.

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart:955: new LabelDefinitionX(null,
'l${index++}', this)..setBreakTarget());
On 2017/01/17 23:33:09, Emily Fortuna wrote:
> On 2017/01/14 03:18:22, sra1 wrote:
> > Figure out how to make the label consistent.
> > Maybe use 'L' instead of 'l'.
> 
> what do you mean by consistent? I made the capitalization change.

I mean that every function starts with L0, rather than having some library
function be compiled with L926 in one app and L678 in another.

https://codereview.chromium.org/2637483002/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2637483002/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:1128: Map<ir.Expression,
ConstantValue> buildSwitchCaseConstants(
can this be private?

https://codereview.chromium.org/2637483002/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:2585: bool previouslyInTrySequence;
Keep 'saved' names consistent, e.g. previousInTryStatement.

https://codereview.chromium.org/2637483002/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart (right):

https://codereview.chromium.org/2637483002/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart:44: final Map<ir.TreeNode,
KernelJumpTarget> _jumpTargets =
Consider adding an interface type in the Kernel (or ask Asger)

https://codereview.chromium.org/2637483002/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/switch_continue_analysis.dart (right):

https://codereview.chromium.org/2637483002/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/switch_continue_analysis.dart:8:
SwitchContinueAnalysis._internal();
'_internal' is fine, but we also often use simply '_'.

[Emily Fortuna, 2017-01-19 00:21:22]

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart (right):

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart:936: ir.Node targetStatement;
On 2017/01/18 23:31:28, sra1 wrote:
> On 2017/01/17 23:33:09, Emily Fortuna wrote:
> > On 2017/01/14 03:18:22, sra1 wrote:
> > > ir.Statement ?
> > 
> > Unfortunately no. This can be a SwitchCase, which is not a statement.
> 
> Ok, perhaps say this in the comment.

Done.

https://codereview.chromium.org/2637483002/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart:955: new LabelDefinitionX(null,
'l${index++}', this)..setBreakTarget());
On 2017/01/18 23:31:28, sra1 wrote:
> On 2017/01/17 23:33:09, Emily Fortuna wrote:
> > On 2017/01/14 03:18:22, sra1 wrote:
> > > Figure out how to make the label consistent.
> > > Maybe use 'L' instead of 'l'.
> > 
> > what do you mean by consistent? I made the capitalization change.
> 
> I mean that every function starts with L0, rather than having some library
> function be compiled with L926 in one app and L678 in another.

Done.

https://codereview.chromium.org/2637483002/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2637483002/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:1128: Map<ir.Expression,
ConstantValue> buildSwitchCaseConstants(
On 2017/01/18 23:31:28, sra1 wrote:
> can this be private?

you bet.

https://codereview.chromium.org/2637483002/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:2585: bool previouslyInTrySequence;
On 2017/01/18 23:31:28, sra1 wrote:
> Keep 'saved' names consistent, e.g. previousInTryStatement.

Done.

https://codereview.chromium.org/2637483002/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart (right):

https://codereview.chromium.org/2637483002/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/kernel_ast_adapter.dart:44: final Map<ir.TreeNode,
KernelJumpTarget> _jumpTargets =
On 2017/01/18 23:31:28, sra1 wrote:
> Consider adding an interface type in the Kernel (or ask Asger)

Done.

https://codereview.chromium.org/2637483002/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/switch_continue_analysis.dart (right):

https://codereview.chromium.org/2637483002/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/switch_continue_analysis.dart:8:
SwitchContinueAnalysis._internal();
On 2017/01/18 23:31:28, sra1 wrote:
> '_internal' is fine, but we also often use simply '_'.

Done.

[Emily Fortuna, 2017-01-19 00:22:09]

Description was changed from

==========
Implement switch statement, without the "complex switch statement" (aka switch
statement with continue to a label).

Breaking this change up into two changes, as there's one last bug I'm tracking
down on switch with continue.

BUG=https://github.com/dart-lang/sdk/issues/27394
==========

to

==========
Implement switch statement, without the "complex switch statement" (aka switch
statement with continue to a label).

Breaking this change up into two changes, as there's one last bug I'm tracking
down on switch with continue.

BUG=https://github.com/dart-lang/sdk/issues/27394
R=sra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/67445d409e2017b5d4592ac03154bd9e0803ff85
==========

[Emily Fortuna, 2017-01-19 00:22:10]

Committed patchset #4 (id:80001) manually as
67445d409e2017b5d4592ac03154bd9e0803ff85 (presubmit successful).