Continue for simple loops (while/for).

Continue for simple loops (while/for).

Not yet implemented for do-while and for-in, or even begun for switch cases.

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

[ngeoffray, 2012-03-19 11:42:06]

LGTM

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

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/resolver.dart#...
frog/leg/resolver.dart:1105: // might have updated its mapping to the target it
actaully does target.
actaully -> actually

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

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:609: // Represents a single break instruction.
break/continue

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:629: // Inert break handler used to avoid null checks
when a loop isn't
Inert -> Insert

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:631: // handler associated with it.
Please update description now that you support switch.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:643: // Breaks are always forward jumps.
Ditto

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:738: // The break handler to use for an upcoming loop
statement (temporarily set
break -> jump. On the other hand, I think you'll get rid of that field.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:1170: List<LocalsHandler> continueLocals =
<LocalsHandler>[];
Please explain why this isn't done in endLoop.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:2182: handler.generateBreak();
I think I'd prefer something like:
handler.generateBreak(node.target === null ? null : elements[node.target]).

It would also get rid of the optional parameter.

It's a matter of style, so your choice.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:2191: assert(!isAborted());
I'd get rid of that assert, since the method that visits a block does not have
an abort block when visiting an individual instruction.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:2204: JumpHandler getLoopJumpHandler(Loop node) {
Please consider what we discussed and remove this function.

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

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:256: }
Shouldn't you do: if !(labeledBlockInfo.labels.isEmpty()) ?

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:473: if (target.isSwitch) {
Please add a comment that since we're generating switch as a loop, we must break
from that loop.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:483: // No block finishing with a 'break' can have
more than
break -> continue

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:484: // one dominated block (since it has only one
successor).
more than one -> a ? You're checking that dominatedBlock.isEmpty.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:488: // Otherwise we would have bailed out in the
builder.
Should that comment be one line above?

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:490: buffer.add("break ");
Please add a comment on why this isn't 'continue'.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:1217: addBreakLabel(label);
What about continue labels here?

[Lasse Reichstein Nielsen, 2012-03-19 12:13:58]

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

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/resolver.dart#...
frog/leg/resolver.dart:1105: // might have updated its mapping to the target it
actaully does target.
On 2012/03/19 11:42:06, ngeoffray wrote:
> actaully -> actually

Done.

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

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:609: // Represents a single break instruction.
On 2012/03/19 11:42:06, ngeoffray wrote:
> break/continue

Done.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:629: // Inert break handler used to avoid null checks
when a loop isn't
On 2012/03/19 11:42:06, ngeoffray wrote:
> Inert -> Insert

Done.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:631: // handler associated with it.
On 2012/03/19 11:42:06, ngeoffray wrote:
> Please update description now that you support switch.

Done.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:643: // Breaks are always forward jumps.
On 2012/03/19 11:42:06, ngeoffray wrote:
> Ditto

Done.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:738: // The break handler to use for an upcoming loop
statement (temporarily set
True, this one can go away too (just as the next one).

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:1170: List<LocalsHandler> continueLocals =
<LocalsHandler>[];
Because endLoop is also used by DoWhile, which isn't handled yet. It'll probably
be moved to endLoop when this is all done.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:2182: handler.generateBreak();
I generally find the conditional operator harder to read than proper if/else.
I'll keep this.
Also, it would probably be a type warning to inline it without "casting"
elements[node.target] to LabelElement.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:2191: assert(!isAborted());
On 2012/03/19 11:42:06, ngeoffray wrote:
> I'd get rid of that assert, since the method that visits a block does not have
> an abort block when visiting an individual instruction.

Done.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/builder.da...
frog/leg/ssa/builder.dart:2204: JumpHandler getLoopJumpHandler(Loop node) {
Renamed to createJumpHandler, and always creates something.
Still used in two places, so I'd like to share the code.

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

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:256: }
It's never empty, so no.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:473: if (target.isSwitch) {
On 2012/03/19 11:42:06, ngeoffray wrote:
> Please add a comment that since we're generating switch as a loop, we must
break
> from that loop.

Done.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:488: // Otherwise we would have bailed out in the
builder.
These comments should all go away, just as they did for break in the "switch"
CL.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:490: buffer.add("break ");
On 2012/03/19 11:42:06, ngeoffray wrote:
> Please add a comment on why this isn't 'continue'.

Done.

https://chromiumcodereview.appspot.com/9718034/diff/1/frog/leg/ssa/codegen.da...
frog/leg/ssa/codegen.dart:1217: addBreakLabel(label);
Continue labels are put as labels on a block surrounding the body, so that we
can break the body to reach the update block.