Don't lose the expected precedence during && compilation.

lib/compiler/implementation/ssa/codegen.dart

Index: lib/compiler/implementation/ssa/codegen.dart
diff --git a/lib/compiler/implementation/ssa/codegen.dart b/lib/compiler/implementation/ssa/codegen.dart
index c4e41ad8737e9a3bd53c236a92bdbae164f91aab..c7ddd09b9506d9e228ed455a4be67c97fba9c8ea 100644
--- a/lib/compiler/implementation/ssa/codegen.dart
+++ b/lib/compiler/implementation/ssa/codegen.dart
@@ -393,6 +393,11 @@ class SsaCodeGenerator implements HVisitor, HBlockInformationVisitor {
   void generateExpression(HExpressionInformation expression) {
     // Currently we only handle sub-expression graphs.
     assert(expression is HSubExpressionBlockInformation);
+    // [visitSubGraph] will reset the [expectedPrecedence]. Make sure we don't
+    // need parenthesis.

[Lasse Reichstein Nielsen, 2012/06/27 08:53:58]

I.e., this only expects to be called for top-level expressions, not
sub-expressions.

[floitsch, 2012/06/27 11:18:24]

Done.

+    assert(expectedPrecedence == JSPrecedence.STATEMENT_PRECEDENCE
+        || expectedPrecedence == JSPrecedence.EXPRESSION_PRECEDENCE);
+
     HSubExpressionBlockInformation expressionSubGraph = expression;
 
     int oldState = generationState;
@@ -571,18 +576,21 @@ class SsaCodeGenerator implements HVisitor, HBlockInformationVisitor {
     if (isGeneratingExpression()) {
       addExpressionSeparator();
     } else {
+      assert(expectedPrecedence == JSPrecedence.STATEMENT_PRECEDENCE);
       addIndentation();
     }
     if (!instruction.isControlFlow() && variableNames.hasName(instruction)) {
       var name = variableNames.getName(instruction);
       if (!handleSimpleUpdateDefinition(instruction, name)
           && !handleTypeConversion(instruction, name)) {
-        declareInstruction(instruction);
-        buffer.add(" = ");
-        visit(instruction, JSPrecedence.ASSIGNMENT_PRECEDENCE);
+        withPrecedence(JSPrecedence.ASSIGNMENT_PRECEDENCE, () {
+          declareInstruction(instruction);
+          buffer.add(" = ");
+          visit(instruction, JSPrecedence.ASSIGNMENT_PRECEDENCE);
+        });
       }
     } else {
-      visit(instruction, JSPrecedence.STATEMENT_PRECEDENCE);
+      visit(instruction, expectedPrecedence);
     }
     if (!isGeneratingExpression()) buffer.add(';\n');
   }
@@ -1111,6 +1119,7 @@ class SsaCodeGenerator implements HVisitor, HBlockInformationVisitor {
       if (instruction is HTypeGuard) {
         visit(instruction, JSPrecedence.STATEMENT_PRECEDENCE);
       } else if (!isGenerateAtUseSite(instruction)) {
+        expectedPrecedence = JSPrecedence.STATEMENT_PRECEDENCE;
         define(instruction);

[Lasse Reichstein Nielsen, 2012/06/27 08:53:58]

Won't a "define" always expect to be at the "statement" level? I.e., maybe move
the assignment into define(..).

[floitsch, 2012/06/27 11:18:24]

No. 'define' is called from visitExpression below. We have cases where we
produce: "foo && (x = bar)".

       }
       instruction = instruction.next;
@@ -1457,6 +1466,32 @@ class SsaCodeGenerator implements HVisitor, HBlockInformationVisitor {
       indent = oldIndent;
     }
 
+    void visitExpression(HStatementInformation toVisit) {
+      // [generateExpression] only works if the [expectedPrecedence] is a
+      // statement or an expression. We therefore have to duplicate some
+      // work here.

[Lasse Reichstein Nielsen, 2012/06/27 08:53:58]

Have you considered making a visitSubGraph that takes an expected precedence
with it. Perhaps make it optional and default to Statement?
Duplicating code because the other code almost does what you want smells like
you should just generalize the other code. It was originally written for a
specific use, but there is nothing wrong with making it more general.

[floitsch, 2012/06/27 11:18:24]

Without the asserts we only have 8 lines here. They are fairly specialized, and
if we need more we should adapt generateExpressions. Until then I think it is
easier to keep it like it is now.

+      assert(toVisit.start == toVisit.end);
+      assert(toVisit.start.last is HGoto);
+      // Find the expression (there must only be one).
+      HInstruction expression = toVisit.start.first;
+      while (generateAtUseSite.contains(expression)) {
+        expression = expression.next;
+      }
+      assert(() {
+        HInstruction remaining = expression.next;
+        while (remaining is !HGoto) {
+          if (generateAtUseSite.contains(remaining)) return false;

[Lasse Reichstein Nielsen, 2012/06/27 08:53:58]

Shouldn't this test be negated?

[floitsch, 2012/06/27 11:18:24]

Done.

+          remaining = remaining.next;
+        }
+        return true;
+      });
+
+      int oldState = generationState;
+      generationState = STATE_FIRST_EXPRESSION;

[Lasse Reichstein Nielsen, 2012/06/27 08:53:58]

x

[floitsch, 2012/06/27 11:18:24]

Done.

+      define(expression);
+      generationState = oldState;
+    }
+
     void visitWithIndent(HStatementInformation toVisit) {
       buffer.add('{\n');
       indent++;
@@ -1479,9 +1514,10 @@ class SsaCodeGenerator implements HVisitor, HBlockInformationVisitor {
       buffer.add(" && ");
       var oldPrecedence = expectedPrecedence;
       expectedPrecedence = operatorPrecedence.right;
-      visitWithoutIndent(toVisit);
+      visitExpression(toVisit);
       expectedPrecedence = oldPrecedence;
       endExpression(operatorPrecedence.precedence);
+      buffer.add(";\n");
     }
 
     List<HBasicBlock> thenSuccessors = thenGraph.end.successors;