Change visitLoop to handleLoop that takes closures as arguments.

frog/leg/ssa/builder.dart

Index: frog/leg/ssa/builder.dart
diff --git a/frog/leg/ssa/builder.dart b/frog/leg/ssa/builder.dart
index 086bc88312d503b6c231df69ca385656e088d155..49f496e96da7ca9d08105d59ffa71f8791909cae 100644
--- a/frog/leg/ssa/builder.dart
+++ b/frog/leg/ssa/builder.dart
@@ -1096,11 +1096,13 @@ class SsaBuilder implements Visitor {
   }
 
   // For while loops, initializer and update are null.
-  visitLoop(Node loop,
-            Node initializer,
-            Expression condition,
-            NodeList updates,
-            Node body) {
+  // The condition function must return a boolean result.

[ngeoffray, 2012/03/21 07:58:47]

Since they'are all doing, why not making it uniform and doing a popBoolified in
this method, and typing the return type as void (like the other parameters)?

[Lasse Reichstein Nielsen, 2012/03/21 09:00:15]

I expect to use this one in switch-with-continue, where the condition will be
"true". I.e., it won't need the boolify.
For that reason I don't want the handleLoop to always add a boolify.

I could make it pass the condition instruction on the stack instead of as a
result, but I actually find this more readable.

+  // None of the functions must leave anything on the stack.
+  handleLoop(Node loop,
+             void initialize(),
+             HInstruction condition(),
+             void update(),
+             void body()) {
     // Generate:
     //  <initializer>
     //  loop-entry:
@@ -1109,19 +1111,12 @@ class SsaBuilder implements Visitor {
     //    <updates>
     //    goto loop-entry;
     //  loop-exit:
-    if (body === null) {
-      compiler.unimplemented(
-          'SsaBuilder.visitLoop with empty body',
-          node: loop);
-    }
 
     localsHandler.startLoop(loop);
 
     // The initializer.
-    if (initializer !== null) {
-      visit(initializer);
-      // We don't care about the value of the initialization.
-      if (initializer.asExpression() !== null) pop();
+    if (initialize !== null) {
+      initialize();
     }
     assert(!isAborted());
 
@@ -1130,8 +1125,7 @@ class SsaBuilder implements Visitor {
 
     HInstruction conditionInstruction;
     if (condition != null) {
-      visit(condition);
-      conditionInstruction = popBoolified();
+      conditionInstruction = condition();
     } else {
       // TODO(ngeoffray): Once our loop recognition does not require a
       // HLoopBranch, we could just generate a HGoto.
@@ -1148,8 +1142,10 @@ class SsaBuilder implements Visitor {
     open(beginBodyBlock);
 
     localsHandler.enterLoopBody(loop);
+    if (body !== null) {
+      hackAroundPossiblyAbortingBody(loop, body);
+    }
 
-    hackAroundPossiblyAbortingBody(body);
     SubGraph bodyGraph = new SubGraph(beginBodyBlock, current);
     HBasicBlock bodyBlock = close(new HGoto());
 
@@ -1181,14 +1177,8 @@ class SsaBuilder implements Visitor {
 
     localsHandler.enterLoopUpdates(loop);
 
-    if (updates !== null) {
-      for (Expression expression in updates) {
-        visit(expression);
-        assert(!isAborted());
-        // The result of the update instruction isn't used, and can just
-        // be dropped.
-        HInstruction updateInstruction = pop();
-      }
+    if (update !== null) {
+      update();
     }
     updateBlock = close(new HGoto());
     // The back-edge completing the cycle.
@@ -1200,11 +1190,39 @@ class SsaBuilder implements Visitor {
 
   visitFor(For node) {
     assert(node.body !== null);
-    visitLoop(node, node.initializer, node.condition, node.update, node.body);
+    void buildInitializer() {
+      Node initializer = node.initializer;
+      if (initializer !== null) {
+        visit(initializer);
+        if (initializer.asExpression() !== null) {
+          pop();
+        }
+      }
+    }
+    HInstruction buildCondition() {
+      visit(node.condition);
+      return popBoolified();
+    }
+    void buildUpdate() {
+      for (Expression expression in node.update) {
+        visit(expression);
+        assert(!isAborted());
+        // The result of the update instruction isn't used, and can just
+        // be dropped.
+        HInstruction updateInstruction = pop();
+      }
+    }
+    handleLoop(node,
+               node.initializer === null ? null : buildInitializer,

[ngeoffray, 2012/03/21 07:58:47]

How about (for this one and all others):

node.initializer === null ? () {} : buildInitializer,

in order to avoid the null checks in handleLoop. There is one extra check for
the condition, but the check is already in handleLoop (and not necessary for a
while and for...in).

[Lasse Reichstein Nielsen, 2012/03/21 09:00:15]

In this case, I could just put the test into the buildInitializer, etc.,
functions and pass them unconditionally.

This is really the only case where parts can be missing, so it's the only case
where we might pass null. As such, it makes sense that this is the place where
we handle that case.

On the other hand, the difference between a while and a for loop is that the for
loop is the only one with an update block. I might want to be able to record
this in the LoopInformation (at some point). But that's being speculative, and
it's easily changable. I'll make the functions all non-null.

+               node.condition === null ? null : buildCondition,
+               node.update.isEmpty() ? null : buildUpdate,
+               () { visit(node.body); });
   }
 
   visitWhile(While node) {
-    visitLoop(node, null, node.condition, null, node.body);
+    handleLoop(node, null, HInstruction condition() { visit(node.condition);

[ngeoffray, 2012/03/21 07:58:47]

I'd prefer one argument per line, or write the condition function as a statement
before. The formatting of the condition parameter looks horrible here.

[Lasse Reichstein Nielsen, 2012/03/21 09:00:15]

Done.

+                                                      return popBoolified(); },
+                     null, () { visit(node.body); });
   }
 
   visitDoWhile(DoWhile node) {
@@ -1213,7 +1231,7 @@ class SsaBuilder implements Visitor {
     HBasicBlock loopEntryBlock = current;
 
     localsHandler.enterLoopBody(node);
-    hackAroundPossiblyAbortingBody(node.body);
+    hackAroundPossiblyAbortingBody(node, () { visit(node.body); });
 
     // If there are no continues we could avoid the creation of the condition
     // block. This could also lead to a block having multiple entries and exits.
@@ -2285,76 +2303,44 @@ class SsaBuilder implements Visitor {
     //     E <declaredIdentifier> = $iter.next();
     //     <body>
     //   }
-    localsHandler.startLoop(node);
-
-    SourceString iteratorName = const SourceString("iterator");
 
+    // All the generated calls are to zero-argument functions.
     Selector selector = Selector.INVOCATION_0;
-    Element interceptor = interceptors.getStaticInterceptor(iteratorName, 0);
-    assert(interceptor != null);
-    HStatic target = new HStatic(interceptor);
-    add(target);
-    visit(node.expression);
-    List<HInstruction> inputs = <HInstruction>[target, pop()];
-    HInstruction iterator = new HInvokeInterceptor(
-        selector, iteratorName, false, inputs);
-    add(iterator);
-
-    JumpHandler jumpHandler = beginLoopHeader(node);
-    HBasicBlock conditionBlock = current;
-
-    // The condition.
-    push(new HInvokeDynamicMethod(
-        selector, const SourceString('hasNext'), [iterator]));
-    HBasicBlock conditionExitBlock = close(new HLoopBranch(popBoolified()));
-
-    LocalsHandler savedLocals = new LocalsHandler.from(localsHandler);
-
-    // The body.
-    HBasicBlock bodyBlock = addNewBlock();
-    conditionExitBlock.addSuccessor(bodyBlock);
-    open(bodyBlock);
-
-    // The call to next is considered to be part of the loop body.
-    localsHandler.enterLoopBody(node);
-
-    push(new HInvokeDynamicMethod(
-        selector, const SourceString('next'), [iterator]));
+    // The iterator is shared between initializer, condition and body.
+    HInstruction iterator;
+    void buildInitialize() {

[ngeoffray, 2012/03/21 07:58:47]

buildInitializer?

[Lasse Reichstein Nielsen, 2012/03/21 09:00:15]

Done.

+      SourceString iteratorName = const SourceString("iterator");
+      Element interceptor = interceptors.getStaticInterceptor(iteratorName, 0);
+      assert(interceptor != null);
+      HStatic target = new HStatic(interceptor);
+      add(target);
+      visit(node.expression);
+      List<HInstruction> inputs = <HInstruction>[target, pop()];
+      iterator = new HInvokeInterceptor(selector, iteratorName, false, inputs);
+      add(iterator);
+    }
+    HInstruction buildCondition() {
+      push(new HInvokeDynamicMethod(
+           selector, const SourceString('hasNext'), [iterator]));

[ngeoffray, 2012/03/21 07:58:47]

<Instruction>[iterator]

[Lasse Reichstein Nielsen, 2012/03/21 09:00:15]

Done.

+      return popBoolified();
+    }
+    void buildBody() {
+      push(new HInvokeDynamicMethod(
+           selector, const SourceString('next'), [iterator]));
+
+      Element variable;
+      if (node.declaredIdentifier.asSend() !== null) {
+        variable = elements[node.declaredIdentifier];
+      } else {
+        assert(node.declaredIdentifier.asVariableDefinitions() !== null);
+        VariableDefinitions variableDefinitions = node.declaredIdentifier;
+        variable = elements[variableDefinitions.definitions.nodes.head];
+      }
+      localsHandler.updateLocal(variable, pop());
 
-    Element variable;
-    if (node.declaredIdentifier.asSend() !== null) {
-      variable = elements[node.declaredIdentifier];
-    } else {
-      assert(node.declaredIdentifier.asVariableDefinitions() !== null);
-      VariableDefinitions variableDefinitions = node.declaredIdentifier;
-      variable = elements[variableDefinitions.definitions.nodes.head];
+      visit(node.body);
     }
-    localsHandler.updateLocal(variable, pop());
-
-    hackAroundPossiblyAbortingBody(node.body);
-    bodyBlock = close(new HGoto());
-
-    jumpHandler.forEachContinue((x,y) {
-      // TODO(lrn): Handle continue in for-in.
-      // TODO(lrn): Or, preferably, use an abstraction of visitLoop for for-in.
-      compiler.cancel('for-in with continue', node: node);
-    });
-
-    // Update.
-    // We create an update block, even if we are in a for-in loop. The
-    // update block is the jump-target for continue statements. We could avoid
-    // the creation if there is no continue, but for now we always create it.
-    HBasicBlock updateBlock = addNewBlock();
-
-    bodyBlock.addSuccessor(updateBlock);
-    open(updateBlock);
-    updateBlock = close(new HGoto());
-    // The back-edge completing the cycle.
-    updateBlock.addSuccessor(conditionBlock);
-    conditionBlock.postProcessLoopHeader();
-
-    endLoop(conditionBlock, conditionExitBlock, jumpHandler, savedLocals);
-    jumpHandler.close();
+    handleLoop(node, buildInitialize, buildCondition, null, buildBody);
   }
 
   visitLabeledStatement(LabeledStatement node) {
@@ -2381,7 +2367,7 @@ class SsaBuilder implements Visitor {
     HBasicBlock entryBlock = graph.addNewBlock();
     goto(current, entryBlock);
     open(entryBlock);
-    hackAroundPossiblyAbortingBody(body);
+    hackAroundPossiblyAbortingBody(node, () { visit(body); });
     SubGraph bodyGraph = new SubGraph(entryBlock, lastOpenedBlock);
 
     HBasicBlock joinBlock = graph.addNewBlock();
@@ -2699,13 +2685,13 @@ class SsaBuilder implements Visitor {
   }
 
   /** HACK HACK HACK */
-  void hackAroundPossiblyAbortingBody(Node body) {
+  void hackAroundPossiblyAbortingBody(Node statement, void body()) {
     stack.add(graph.addConstantBool(true));
     buildBody() {
       // TODO(lrn): Make sure to take continue into account.
-      visit(body);
+      body();
       if (isAborted()) {
-        compiler.reportWarning(body, "aborting loop body");
+        compiler.reportWarning(statement, "aborting loop body");
       }
     }
     handleIf(buildBody, null);