Accept more labels per switch case.

Accept more labels per switch case.

This matches the new, disambiguated switch syntax.

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

[karlklose, 2012-05-11 09:29:31]

https://chromiumcodereview.appspot.com/10387080/diff/1/frog/tests/leg_only/sw...
File frog/tests/leg_only/switch_test.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/1/frog/tests/leg_only/sw...
frog/tests/leg_only/switch_test.dart:60: // Tests some badly formed switch
bodies.
Tests -> Test.

https://chromiumcodereview.appspot.com/10387080/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:1214: print(target);
Remove debug code.

https://chromiumcodereview.appspot.com/10387080/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:1246: 
Remove empty line.

[Lasse Reichstein Nielsen, 2012-05-11 14:17:09]

Peter, please take another look.

https://chromiumcodereview.appspot.com/10387080/diff/1/frog/tests/leg_only/sw...
File frog/tests/leg_only/switch_test.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/1/frog/tests/leg_only/sw...
frog/tests/leg_only/switch_test.dart:60: // Tests some badly formed switch
bodies.
On 2012/05/11 09:29:31, karlklose wrote:
> Tests -> Test.

Done.

https://chromiumcodereview.appspot.com/10387080/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:1214: print(target);
On 2012/05/11 09:29:31, karlklose wrote:
> Remove debug code.

Done.

https://chromiumcodereview.appspot.com/10387080/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:1246: 
On 2012/05/11 09:29:31, karlklose wrote:
> Remove empty line.

Done.

[Lasse Reichstein Nielsen, 2012-05-11 14:17:09]

Peter, please take another look.

https://chromiumcodereview.appspot.com/10387080/diff/1/frog/tests/leg_only/sw...
File frog/tests/leg_only/switch_test.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/1/frog/tests/leg_only/sw...
frog/tests/leg_only/switch_test.dart:60: // Tests some badly formed switch
bodies.
On 2012/05/11 09:29:31, karlklose wrote:
> Tests -> Test.

Done.

https://chromiumcodereview.appspot.com/10387080/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:1214: print(target);
On 2012/05/11 09:29:31, karlklose wrote:
> Remove debug code.

Done.

https://chromiumcodereview.appspot.com/10387080/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:1246: 
On 2012/05/11 09:29:31, karlklose wrote:
> Remove empty line.

Done.

[Lasse Reichstein Nielsen, 2012-05-11 14:27:52]

I changed the SwitchCase to have a single list of both labels and
case-expressions. It made both parsing and unparsing easier, but takes a litte
more effort when traversing in the resolver and builder.

[ahe, 2012-05-14 09:24:25]

LGTM!

https://chromiumcodereview.appspot.com/10387080/diff/9001/frog/tests/leg_only...
File frog/tests/leg_only/switch_test.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/9001/frog/tests/leg_only...
frog/tests/leg_only/switch_test.dart:36: break fubar;
That is a cuss word. It means "F'ed up beyond all recognition/reason". Perhaps
choose a different word.

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1244: visitLabel(Label node) {
Perhaps add a comment to explain why this is empty?

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
lib/compiler/implementation/scanner/listener.dart:1357: Expect.isTrue(switchCase
is SwitchCase);
Please avoid assertions, an internal error has more information for the user
(and compiler engineer).

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/scanner/parser.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
lib/compiler/implementation/scanner/parser.dart:1566: Token peekPastLabels(Token
peek) {
I would prefer if this function's argument was named "token" as this is
generally the convention in the parser.

I think it would be cleaner if this wasn't a nested function, for several
reasons:

* You can follow the "token" convention without hiding anything.

* This function could be useful for an editor.

* Porting this to Java is annoying. Brian from the Editor team is currently
doing so and one of the reasons is that I have claimed the parser uses no
"fancy" features—fancy meaning "not in Java" ;-)

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
lib/compiler/implementation/scanner/parser.dart:1567: Token pre = peek;
Unused variable?

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
lib/compiler/implementation/scanner/parser.dart:1576: Token peek;
It is confusing that peekPastLabels has "peek" parameter and this temporary
exists.

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
lib/compiler/implementation/scanner/parser.dart:1582: Token
parseSwitchCase(Token token) {
This method is definitely something that I think a user of the parser would like
access to. So it is a problem that it is not a pure function (state in peek). I
would like to examine if it is possible to avoid that.

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:3038: unreachable();
Please report an error that the user can understand.

[ahe, 2012-05-14 09:31:24]

A few follow comments on the parser changes.

https://chromiumcodereview.appspot.com/10387080/diff/6013/lib/compiler/implem...
File lib/compiler/implementation/scanner/parser.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/6013/lib/compiler/implem...
lib/compiler/implementation/scanner/parser.dart:1618: while (token.kind !==
EOF_TOKEN) {
I dont' think the parser reports errors for this:

main() {
  switch (1) {
  case 1:
    foo();
// EOF

https://chromiumcodereview.appspot.com/10387080/diff/6013/lib/compiler/implem...
lib/compiler/implementation/scanner/parser.dart:1620: if (value === 'case' ||
value === 'default' ||
Please add parentheses around these expressions. I cannot read short-circuit
boolean expressions without them.

https://chromiumcodereview.appspot.com/10387080/diff/6013/lib/compiler/implem...
lib/compiler/implementation/scanner/parser.dart:1630:
listener.handleSwitchCase(labelCount, expressionCount, defaultKeyword,
Should labelCount and expressionCount be added together?

[Lasse Reichstein Nielsen, 2012-05-14 10:34:57]

https://chromiumcodereview.appspot.com/10387080/diff/9001/frog/tests/leg_only...
File frog/tests/leg_only/switch_test.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/9001/frog/tests/leg_only...
frog/tests/leg_only/switch_test.dart:36: break fubar;
Or "beyond all repair", even.
Will change.

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1244: visitLabel(Label node) {
On 2012/05/14 09:24:25, ahe wrote:
> Perhaps add a comment to explain why this is empty?

Done.

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
lib/compiler/implementation/scanner/listener.dart:1357: Expect.isTrue(switchCase
is SwitchCase);
Ack. Leftover debug code.

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/scanner/parser.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
lib/compiler/implementation/scanner/parser.dart:1566: Token peekPastLabels(Token
peek) {
It's now a top-level function. I'll rename "peek" to "token".

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
lib/compiler/implementation/scanner/parser.dart:1567: Token pre = peek;
On 2012/05/14 09:24:25, ahe wrote:
> Unused variable?

Done.

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
lib/compiler/implementation/scanner/parser.dart:1576: Token peek;
It was actually deliberate. Alas, parameter is now "token".

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
lib/compiler/implementation/scanner/parser.dart:1582: Token
parseSwitchCase(Token token) {
It's avoided, but at the cost of potentially peeking past the same labels twice.
It's expected that labels on cases are rare, and multiple labels even more so,
so it's probably not worth this overengineering to avoid it.

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:3038: unreachable();
Made in6o internal error. DItto for the one below.

https://chromiumcodereview.appspot.com/10387080/diff/6013/lib/compiler/implem...
File lib/compiler/implementation/scanner/parser.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/6013/lib/compiler/implem...
lib/compiler/implementation/scanner/parser.dart:1618: while (token.kind !==
EOF_TOKEN) {
It generates an "unbalanced '{'" error.

https://chromiumcodereview.appspot.com/10387080/diff/6013/lib/compiler/implem...
lib/compiler/implementation/scanner/parser.dart:1620: if (value === 'case' ||
value === 'default' ||
Done.
Maybe putting them on separate lines is better?
I find it harder to read with too many parentheses.

https://chromiumcodereview.appspot.com/10387080/diff/6013/lib/compiler/implem...
lib/compiler/implementation/scanner/parser.dart:1630:
listener.handleSwitchCase(labelCount, expressionCount, defaultKeyword,
I prefer letting the listener decide that.

[ahe, 2012-05-14 10:44:09]

https://chromiumcodereview.appspot.com/10387080/diff/6013/lib/compiler/implem...
File lib/compiler/implementation/scanner/parser.dart (right):

https://chromiumcodereview.appspot.com/10387080/diff/6013/lib/compiler/implem...
lib/compiler/implementation/scanner/parser.dart:1620: if (value === 'case' ||
value === 'default' ||
On 2012/05/14 10:34:58, Lasse Reichstein Nielsen wrote:
> Done.
> Maybe putting them on separate lines is better?
> I find it harder to read with too many parentheses.

Seriously: I do not understand what it means. This is not about readability, I
do not understand the code without parentheses.