Switch-implementation.

Switch-implementation.

Change parser to compile several case expressions into one SwitchCase - so there is one case per statement group.

Make the builder generate a structure similar to what would be created by an if/else-if cascade with logically or'ed comparisons as conditions.

Not handling continues (but then, nothing is yet).

Committed: https://code.google.com/p/dart/source/detail?r=5389

[Lasse Reichstein Nielsen, 2012-03-08 12:10:13]

Preliminary peek.
Break inside switch doesn't work yet (I need to either wrap the if-statement in
something breakable, or add a label).

[floitsch, 2012-03-08 16:35:13]

FYI.

https://chromiumcodereview.appspot.com/9632018/diff/1/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/1/frog/leg/resolver.dart#...
frog/leg/resolver.dart:518: Link<StatementElement> breakTargetStack;
These (the break and the continueTargetStack) are only for breaks and continues
without ids. correct?
if yes, please add a comment.
If not, please please add a comment.

https://chromiumcodereview.appspot.com/9632018/diff/1/frog/leg/scanner/listen...
File frog/leg/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/1/frog/leg/scanner/listen...
frog/leg/scanner/listener.dart:1309: if (switchCase.isDefaultCase && !isLast) {
Afaik default can be anywhere. Does not need to be the last.

https://chromiumcodereview.appspot.com/9632018/diff/1/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:2358: // previous is a boolean instruction.
P

[Lasse Reichstein Nielsen, 2012-03-08 18:35:40]

https://chromiumcodereview.appspot.com/9632018/diff/1/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/1/frog/leg/resolver.dart#...
frog/leg/resolver.dart:518: Link<StatementElement> breakTargetStack;
Yes. Will add comment.

https://chromiumcodereview.appspot.com/9632018/diff/1/frog/leg/scanner/listen...
File frog/leg/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/1/frog/leg/scanner/listen...
frog/leg/scanner/listener.dart:1309: if (switchCase.isDefaultCase && !isLast) {
Spec says it must be the last last "label" of the last case.

https://chromiumcodereview.appspot.com/9632018/diff/1/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:2358: // previous is a boolean instruction.
Whoops, 'previous'->'left'.

[ahe, 2012-03-08 18:54:46]

Initial comments.

https://chromiumcodereview.appspot.com/9632018/diff/1/frog/leg/scanner/listen...
File frog/leg/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/1/frog/leg/scanner/listen...
frog/leg/scanner/listener.dart:1309: if (switchCase.isDefaultCase && !isLast) {
On 2012/03/08 18:35:40, Lasse Reichstein Nielsen wrote:
> Spec says it must be the last last "label" of the last case.

The ANTLR grammar agrees:

    | SWITCH '(' expression ')' '{' switchCase* defaultCase? '}'

[Lasse Reichstein Nielsen, 2012-03-09 12:26:13]

Now complete, please review in full.

[ahe, 2012-03-09 13:58:32]

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/elements/el...
File frog/leg/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/elements/el...
frog/leg/elements/elements.dart:866: Node parseNode(DiagnosticListener l) =>
statement;
Unrelated note: I think it would be nice if you override toString and position.
Also for label element.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/lib/js_help...
File frog/leg/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/lib/js_help...
frog/leg/lib/js_helper.dart:1374: getFallThroughError() {
Add documentation comment that explain who calls this.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/resolver.da...
frog/leg/resolver.dart:490: 
Remove extra line

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/resolver.da...
frog/leg/resolver.dart:502: 
Extra line.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/resolver.da...
frog/leg/resolver.dart:598: if (statement is SwitchStatement) {
statement.asSwitchStatement() !== null

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/scanner/lis...
File frog/leg/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/scanner/lis...
frog/leg/scanner/listener.dart:1312: recoverableError("default not on last
switch case",
As we discussed, I'd prefer pushing this check to the resolver.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/scanner/par...
File frog/leg/scanner/parser.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/scanner/par...
frog/leg/scanner/parser.dart:1546: token = expect('case', token);
Where do you store the case token?

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/scanner/par...
frog/leg/scanner/parser.dart:1551: } while (tokenValue === 'case' || tokenValue
=== 'default');
What happens if I write:

switch (expr) {
default:
default:
case foo:
  print('fisk'); break;
case bar:
  print('hest'); break;
}

[floitsch, 2012-03-09 16:52:14]

LGTM after you have addressed peter's and my comments.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/ssa/builder...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:2269: new HLabeledBlockInformation(
keep on one line.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:2320: void buildCompare(Expression caseExpression) {
Can't you move buildCompare and buildThrow into buildSwitchCases?
The only thing that would be needed to be passed to buildSwitchCase would be the
'expression', which seems logical.
Also you could inline buildThrow (it is only used in one place).
I would also consider inlining buildCompare, but we can definitely keep it.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:2323: }
please add a newline after function declarations.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:2397: HInstruction buildTests(Link<Node> expressions,
HInstruction left) {
This looks a lot like the visitLogicalAndOr. I vote for having a handleAndOr
like the handleIf.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:2398: // previous is a boolean instruction.
'previous' is still here.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/tree/unpars...
File frog/leg/tree/unparser.dart (left):

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/tree/unpars...
frog/leg/tree/unparser.dart:343: visitDefaultCase(DefaultCase node) {
Where is the default: case now handled?

[Lasse Reichstein Nielsen, 2012-03-12 13:05:23]

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/elements/el...
File frog/leg/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/elements/el...
frog/leg/elements/elements.dart:866: Node parseNode(DiagnosticListener l) =>
statement;
On 2012/03/09 13:58:32, ahe wrote:
> Unrelated note: I think it would be nice if you override toString and
position.
> Also for label element.

Done.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/elements/el...
frog/leg/elements/elements.dart:869: class SwitchStatementElement extends
StatementElement {
Folded this into StatementElement, using nesting-level of statement as "unique"
key (since it's only to avoid nested labels with the same name).

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/lib/js_help...
File frog/leg/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/lib/js_help...
frog/leg/lib/js_helper.dart:1374: getFallThroughError() {
On 2012/03/09 13:58:32, ahe wrote:
> Add documentation comment that explain who calls this.

Done.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/resolver.da...
frog/leg/resolver.dart:490: 
On 2012/03/09 13:58:32, ahe wrote:
> Remove extra line

Done.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/resolver.da...
frog/leg/resolver.dart:502: 
On 2012/03/09 13:58:32, ahe wrote:
> Extra line.

Done.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/resolver.da...
frog/leg/resolver.dart:598: if (statement is SwitchStatement) {
Why use something that's harder to read and is possibly less efficient?
I.e., BUT WHY?!?!?

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/scanner/lis...
File frog/leg/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/scanner/lis...
frog/leg/scanner/listener.dart:1312: recoverableError("default not on last
switch case",
Done. The only problem is that I can't report the error on a Token there, only
on a Node.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/scanner/par...
File frog/leg/scanner/parser.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/scanner/par...
frog/leg/scanner/parser.dart:1546: token = expect('case', token);
I don't.
You can find it, if you need it, from the begin Token, by iterating forward. We
know it's just before the 
I'll add a function that returns the case keywords if anyone needs them.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/scanner/par...
frog/leg/scanner/parser.dart:1551: } while (tokenValue === 'case' || tokenValue
=== 'default');
The "default:" always ends the case, so this is equivalent to
  default: {}
  default: {}
  case foo: ...
  case bar: ...
I.e., two default cases with fallthrough, and both default keywords will be
considered errors, since only the last case may have one.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/ssa/builder...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:2320: void buildCompare(Expression caseExpression) {
I'll inline them.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:2323: }
On 2012/03/09 16:52:15, floitsch wrote:
> please add a newline after function declarations.

Done.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:2397: HInstruction buildTests(Link<Node> expressions,
HInstruction left) {
Good idea.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:2398: // previous is a boolean instruction.
Not any more. And good riddance.

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/tree/unpars...
File frog/leg/tree/unparser.dart (left):

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/tree/unpars...
frog/leg/tree/unparser.dart:343: visitDefaultCase(DefaultCase node) {
A switch case now contains more than one expression and an optional default flag
and some statements, instead of one case being either one expression or default
and possibly no statements.

[Lasse Reichstein Nielsen, 2012-03-12 13:21:27]

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/7001/frog/leg/resolver.da...
frog/leg/resolver.dart:598: if (statement is SwitchStatement) {
But I rewrote it anyway :)

[ahe, 2012-03-12 13:24:47]

Initial comments. I like the changes to listener.dart and parser.dart.

https://chromiumcodereview.appspot.com/9632018/diff/15015/frog/leg/scanner/li...
File frog/leg/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/15015/frog/leg/scanner/li...
frog/leg/scanner/listener.dart:1307: Link<Node> nodes = const EmptyLink<Node>();
Can't you use makeNodeList now?

https://chromiumcodereview.appspot.com/9632018/diff/15015/frog/leg/scanner/pa...
File frog/leg/scanner/parser.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/15015/frog/leg/scanner/pa...
frog/leg/scanner/parser.dart:1533: label = token;
This is not the label, it is the colon.

https://chromiumcodereview.appspot.com/9632018/diff/15015/frog/leg/scanner/pa...
frog/leg/scanner/parser.dart:1539: String tokenValue = token.stringValue;
Nit: in many other places, I have just used "value" for this purpose.

[floitsch, 2012-03-12 13:46:04]

LGTM once you have addressed Peter's comments.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/ssa/builde...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:2330: if (!isAborted()) {
Please add comment when this happens: it's never aborted, unless there is a
default clause and the default-clause breaks or returns.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:2364: compiler.reportWarning(node, 'Missing break at
end of switch case');
Shouldn't that be caught earlier?
If yes add at least a TODO.

[ahe, 2012-03-13 10:17:15]

LGTM!

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/elements/e...
File frog/leg/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/elements/e...
frog/leg/elements/elements.dart:878: (statement is SwitchStatement) ?
'\$$nestingLevel' : null;
I think this belongs in the SSA builder.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/lib/js_hel...
File frog/leg/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/lib/js_hel...
frog/leg/lib/js_helper.dart:1381: 
Extra line.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/lib/js_hel...
frog/leg/lib/js_helper.dart:1382: // Called at the end of unaborted switch cases
to get the (unique)
Documentation comment.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/lib/js_hel...
frog/leg/lib/js_helper.dart:1382: // Called at the end of unaborted switch cases
to get the (unique)
Did you mean "singleton" instead of "unique"?

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/lib/js_hel...
frog/leg/lib/js_helper.dart:1386: // TODO(lrn): Redo when we support "const
FallThroughError()".
I think this is done.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/resolver.d...
frog/leg/resolver.dart:427: final Map<String, LabelElement> caseLabels;
Add an empty line.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/resolver.d...
frog/leg/resolver.dart:428: SwitchLabelScope(this.outer, this.caseLabels);
Add an empty line.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/resolver.d...
frog/leg/resolver.dart:490: nestingLevel++;
This seems wrong, but perhaps I'm misunderstanding something. Entering a switch
statement does not affect continue, it only affects break. So isn't this
problematic?

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/resolver.d...
frog/leg/resolver.dart:1005: return;
Thank you :-)

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/resolver.d...
frog/leg/resolver.dart:1096: StatementElement breakElement =
getOrCreateStatementElement(node);
Nit: I'm not digging this method. I would prefer if it always created a
statement element.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/resolver.d...
frog/leg/resolver.dart:1127: new LabelElement(labelIdentifier, labelName,
I don't understand the mental model for this. The label is enclosing the
statement, yet the statementElement doesn't see the label element in its chain
of enclosing elements.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/resolver.d...
frog/leg/resolver.dart:1142: continueLabels.forEach((String key, LabelElement
label) {
Wouldn't it better to set a bit on the label?

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/ssa/builde...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:618: interface BreakHandler default BreakHandlerImpl {
Do we really need an interface for this?

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:623: List<LabelElement> labels();
I like seeing a move from SourceString to Element.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:2379: compiler.internalError("Case with no expression
and not default");
Please pass the node.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:2429: unreachable();
Please make sure to pass in the node to unreachable.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/tree/nodes...
File frog/leg/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1415: class SwitchCase extends Node {
A short comment would be helpful, something like:

(label:)? (case: expression)* (default:)? statements;

Each expression is collected in [expressions]; [caseKeywords] can be used to
obtain the begin tokens of each of the cases.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1456: List<Token> caseKeywords() {
Please return a Link for consistency.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1459: // Skip past <Identifier> ':'.
// Skip past the label: <Identifier> ':'.

[Lasse Reichstein Nielsen, 2012-03-13 12:09:38]

I'll address Peters last comments in a different CL.

https://chromiumcodereview.appspot.com/9632018/diff/15015/frog/leg/scanner/pa...
File frog/leg/scanner/parser.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/15015/frog/leg/scanner/pa...
frog/leg/scanner/parser.dart:1533: label = token;
Ack, yes. Good thing I'm not using it for anything except checking whether to
pop the Identifier.

https://chromiumcodereview.appspot.com/9632018/diff/15015/frog/leg/scanner/pa...
frog/leg/scanner/parser.dart:1539: String tokenValue = token.stringValue;
Seems reasonable here too. Will change.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/elements/e...
File frog/leg/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/elements/e...
frog/leg/elements/elements.dart:878: (statement is SwitchStatement) ?
'\$$nestingLevel' : null;
I'll move it somewhere else.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/lib/js_hel...
File frog/leg/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/lib/js_hel...
frog/leg/lib/js_helper.dart:1381: 
On 2012/03/13 10:17:15, ahe wrote:
> Extra line.

Done.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/lib/js_hel...
frog/leg/lib/js_helper.dart:1382: // Called at the end of unaborted switch cases
to get the (unique)
Done.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/lib/js_hel...
frog/leg/lib/js_helper.dart:1382: // Called at the end of unaborted switch cases
to get the (unique)
Singleton is fine too.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/lib/js_hel...
frog/leg/lib/js_helper.dart:1386: // TODO(lrn): Redo when we support "const
FallThroughError()".
Let's see if it works now.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/resolver.d...
frog/leg/resolver.dart:427: final Map<String, LabelElement> caseLabels;
On 2012/03/13 10:17:15, ahe wrote:
> Add an empty line.

Done.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/resolver.d...
frog/leg/resolver.dart:428: SwitchLabelScope(this.outer, this.caseLabels);
On 2012/03/13 10:17:15, ahe wrote:
> Add an empty line.

Done.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/resolver.d...
frog/leg/resolver.dart:490: nestingLevel++;
The switch level doesn't need to be exact, as long as it's different for two
break/continue-target statements if one is inside the other.
I'll add a comment.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/resolver.d...
frog/leg/resolver.dart:1096: StatementElement breakElement =
getOrCreateStatementElement(node);
The problem is that we want to create a StatementElement for a statement as soon
as we know that it's a potential break target.
For a labeled statement, that happens when we encounter the LabelledStatement.
If the statement happens to be a switch or loop, we want to reuse that
StatementElement when we get to the switch/loop.

So this function checks if a StatementElement is already attached to the Node,
if so, returns that, and if not, creates one.
Suggestions for another approach?

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/resolver.d...
frog/leg/resolver.dart:1127: new LabelElement(labelIdentifier, labelName,
Are you referring to enclosingElement? I have assumed that this refers to the
function, not to syntax inside the function.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/resolver.d...
frog/leg/resolver.dart:1142: continueLabels.forEach((String key, LabelElement
label) {
The bit is set. This is pure cleanup. We know that we won't be needing these
labels and statement elements after this, since they are unreferenced. 
We could just let it all stay, and check flags later, but that's a waste of
memory.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/ssa/builde...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:618: interface BreakHandler default BreakHandlerImpl {
Maybe not, but I like making the NullBreakHandler const-creatable.
This is really just a linked stack of break handlers, that manage their own
attach/detach logic.
The NullBreakHandler is just convenience, to avoid checking for null.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:2330: if (!isAborted()) {
On 2012/03/12 13:46:05, floitsch wrote:
> Please add comment when this happens: it's never aborted, unless there is a
> default clause and the default-clause breaks or returns.

Done.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:2364: compiler.reportWarning(node, 'Missing break at
end of switch case');
How? This is the first time we actually detect whether control reaches the end
of the case.
We don't want to warn on:
 case 42: {if (x) return 42; else break;}
so we would a general analysis to check whether a block always aborts.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:2379: compiler.internalError("Case with no expression
and not default");
On 2012/03/13 10:17:15, ahe wrote:
> Please pass the node.

Done.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:2429: unreachable();
I dont' think unreachable takes an argument (defined in leg.dart).

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/tree/nodes...
File frog/leg/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1456: List<Token> caseKeywords() {
On 2012/03/13 10:17:15, ahe wrote:
> Please return a Link for consistency.

Done.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1459: // Skip past <Identifier> ':'.
On 2012/03/13 10:17:15, ahe wrote:
> // Skip past the label: <Identifier> ':'.

Done.

https://chromiumcodereview.appspot.com/9632018/diff/12019/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1466: token = token.next.next.next;
Oh, and this doesn't actually work. Fixing.

[ahe, 2012-03-14 23:08:38]

Hi Lasse,

I didn't notice this earlier. I don't mind filing a bug myself or updating the
status file, but it would be great if you could just reply to this email with
whatever details you can remember.

Cheers,
Peter

https://chromiumcodereview.appspot.com/9632018/diff/19003/tests/co19/co19-leg...
File tests/co19/co19-leg.status (right):

https://chromiumcodereview.appspot.com/9632018/diff/19003/tests/co19/co19-leg...
tests/co19/co19-leg.status:44: Language/11_Statements/11_Labels_A01_t03: Fail,
OK # Test doesn't follow spec.
I plan to to file bugs about these issues at code.google.com/p/co19. I put in
notes that tell me what is wrong so I know what to write in the bug reports.

So I would really appreciate if you could explain in what way this test doesn't
follow the specification or file an issue (chances are that you can't fit the
details in one line).

[Lasse Reichstein Nielsen, 2012-03-19 13:43:21]

https://chromiumcodereview.appspot.com/9632018/diff/19003/tests/co19/co19-leg...
File tests/co19/co19-leg.status (right):

https://chromiumcodereview.appspot.com/9632018/diff/19003/tests/co19/co19-leg...
tests/co19/co19-leg.status:44: Language/11_Statements/11_Labels_A01_t03: Fail,
OK # Test doesn't follow spec.
The error here is that the test claims that it's an error if a statement has
more than one label. The spec grammar allows more than one label on a statement.

I already filed a bug: http://code.google.com/p/dart/issues/detail?id=2130