Fixes a bug where temp local declarations could sometimes end up in a for statement's increments list. Also adds Context.isLvalue() to the Java AST, mirroring the JsContext from the JS AST. http://gwt-code-reviews.appspot.com/677801/show Review by: bowdidge@google.com, spoon git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@8368 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/Context.java b/dev/core/src/com/google/gwt/dev/jjs/ast/Context.java index 6d9b828..b8643d9 100644 --- a/dev/core/src/com/google/gwt/dev/jjs/ast/Context.java +++ b/dev/core/src/com/google/gwt/dev/jjs/ast/Context.java
@@ -30,6 +30,8 @@ void insertBefore(JNode node); + boolean isLvalue(); + void removeMe(); void replaceMe(JNode node);
diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JBinaryOperation.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JBinaryOperation.java index 0ad22c3..3f17ba6 100644 --- a/dev/core/src/com/google/gwt/dev/jjs/ast/JBinaryOperation.java +++ b/dev/core/src/com/google/gwt/dev/jjs/ast/JBinaryOperation.java
@@ -74,7 +74,11 @@ public void traverse(JVisitor visitor, Context ctx) { if (visitor.visit(this, ctx)) { - lhs = visitor.accept(lhs); + if (isAssignment()) { + lhs = visitor.acceptLvalue(lhs); + } else { + lhs = visitor.accept(lhs); + } rhs = visitor.accept(rhs); } visitor.endVisit(this, ctx);
diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JDeclarationStatement.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JDeclarationStatement.java index 36b91ad..10fb994 100644 --- a/dev/core/src/com/google/gwt/dev/jjs/ast/JDeclarationStatement.java +++ b/dev/core/src/com/google/gwt/dev/jjs/ast/JDeclarationStatement.java
@@ -44,7 +44,7 @@ public void traverse(JVisitor visitor, Context ctx) { if (visitor.visit(this, ctx)) { - variableRef = (JVariableRef) visitor.accept(variableRef); + variableRef = (JVariableRef) visitor.acceptLvalue(variableRef); if (initializer != null) { initializer = visitor.accept(initializer); }
diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JModVisitor.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JModVisitor.java index 41106de..9452e33 100644 --- a/dev/core/src/com/google/gwt/dev/jjs/ast/JModVisitor.java +++ b/dev/core/src/com/google/gwt/dev/jjs/ast/JModVisitor.java
@@ -60,6 +60,10 @@ ctxDidChange = true; } + public boolean isLvalue() { + return false; + } + public void removeMe() { checkState(); list.remove(index--); @@ -138,6 +142,10 @@ ctxDidChange = true; } + public boolean isLvalue() { + return false; + } + public void removeMe() { checkState(); list = Lists.remove(list, index--); @@ -181,6 +189,17 @@ } } + private class LvalueContext extends NodeContext { + public LvalueContext() { + super(false); + } + + @Override + public boolean isLvalue() { + return true; + } + } + private static class NodeContext implements Context { boolean canRemove; boolean didChange; @@ -190,7 +209,7 @@ public NodeContext(boolean canRemove) { this.canRemove = canRemove; } - + public boolean canInsert() { return false; } @@ -207,11 +226,15 @@ throw new UnsupportedOperationException("Can't insert before " + node); } + public boolean isLvalue() { + return false; + } + public void removeMe() { if (!canRemove) { throw new UnsupportedOperationException("Can't remove " + node); } - + this.node = null; didChange = true; } @@ -242,7 +265,7 @@ public JNode accept(JNode node) { return accept(node, false); } - + @Override public JNode accept(JNode node, boolean allowRemove) { NodeContext ctx = new NodeContext(allowRemove); @@ -295,6 +318,18 @@ } } + public JExpression acceptLvalue(JExpression expr) { + LvalueContext ctx = new LvalueContext(); + try { + ctx.node = expr; + traverse(expr, ctx); + didChange |= ctx.didChange; + return (JExpression) ctx.node; + } catch (Throwable e) { + throw translateException(expr, e); + } + } + @Override public <T extends JNode> void acceptWithInsertRemove(List<T> list) { new ListContext<T>(list).traverse();
diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JUnaryOperation.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JUnaryOperation.java index 4334938..6430673 100644 --- a/dev/core/src/com/google/gwt/dev/jjs/ast/JUnaryOperation.java +++ b/dev/core/src/com/google/gwt/dev/jjs/ast/JUnaryOperation.java
@@ -50,6 +50,10 @@ } public void traverse(JVisitor visitor, Context ctx) { - arg = visitor.accept(arg); + if (getOp().isModifying()) { + arg = visitor.acceptLvalue(arg); + } else { + arg = visitor.accept(arg); + } } }
diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JVisitor.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JVisitor.java index c31c2f2..0714d76 100644 --- a/dev/core/src/com/google/gwt/dev/jjs/ast/JVisitor.java +++ b/dev/core/src/com/google/gwt/dev/jjs/ast/JVisitor.java
@@ -32,6 +32,37 @@ @SuppressWarnings("unused") public class JVisitor { + protected static final Context LVALUE_CONTEXT = new Context() { + + public boolean canInsert() { + return false; + } + + public boolean canRemove() { + return false; + } + + public void insertAfter(JNode node) { + throw new UnsupportedOperationException(); + } + + public void insertBefore(JNode node) { + throw new UnsupportedOperationException(); + } + + public boolean isLvalue() { + return true; + } + + public void removeMe() { + throw new UnsupportedOperationException(); + } + + public void replaceMe(JNode node) { + throw new UnsupportedOperationException(); + } + }; + protected static final Context UNMODIFIABLE_CONTEXT = new Context() { public boolean canInsert() { @@ -50,6 +81,10 @@ throw new UnsupportedOperationException(); } + public boolean isLvalue() { + return false; + } + public void removeMe() { throw new UnsupportedOperationException(); } @@ -83,7 +118,7 @@ public JNode accept(JNode node) { return accept(node, false); } - + public JNode accept(JNode node, boolean allowRemove) { try { node.traverse(this, UNMODIFIABLE_CONTEXT); @@ -117,6 +152,15 @@ return list; } + public JExpression acceptLvalue(JExpression expr) { + try { + expr.traverse(this, LVALUE_CONTEXT); + return expr; + } catch (Throwable e) { + throw translateException(expr, e); + } + } + public <T extends JNode> void acceptWithInsertRemove(List<T> list) { accept(list); }
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/TempLocalVisitor.java b/dev/core/src/com/google/gwt/dev/jjs/impl/TempLocalVisitor.java index 91737df..1c4deec 100644 --- a/dev/core/src/com/google/gwt/dev/jjs/impl/TempLocalVisitor.java +++ b/dev/core/src/com/google/gwt/dev/jjs/impl/TempLocalVisitor.java
@@ -19,6 +19,7 @@ import com.google.gwt.dev.jjs.ast.Context; import com.google.gwt.dev.jjs.ast.JBlock; import com.google.gwt.dev.jjs.ast.JDeclarationStatement; +import com.google.gwt.dev.jjs.ast.JForStatement; import com.google.gwt.dev.jjs.ast.JLocal; import com.google.gwt.dev.jjs.ast.JLocalRef; import com.google.gwt.dev.jjs.ast.JMethodBody; @@ -28,10 +29,12 @@ import com.google.gwt.dev.jjs.ast.JType; import com.google.gwt.dev.jjs.ast.JVariable; import com.google.gwt.dev.jjs.ast.JVisitor; +import com.google.gwt.dev.util.collect.HashSet; import java.util.BitSet; import java.util.HashMap; import java.util.Map; +import java.util.Set; import java.util.Stack; /** @@ -204,6 +207,12 @@ */ private static final String PREFIX = "$t"; + /** + * A set of statements we cannot insert declaration statements into. Currently + * this is just the "increments" list of a JForStatement. + */ + private Set<JStatement> banList = new HashSet<JStatement>(); + private JMethodBody curMethodBody = null; private Scope curScope = null; private final Stack<Context> insertionStack = new Stack<Context>(); @@ -225,8 +234,10 @@ @Override public final void endVisit(JStatement x, Context ctx) { if (ctx.canInsert()) { - Context popped = insertionStack.pop(); - assert popped == ctx; + if (!banList.remove(x)) { + Context popped = insertionStack.pop(); + assert popped == ctx; + } } super.endVisit(x, ctx); } @@ -247,9 +258,14 @@ @Override public final boolean visit(JStatement x, Context ctx) { - if (ctx.canInsert()) { + if (ctx.canInsert() && !banList.contains(x)) { insertionStack.push(ctx); } + if (x instanceof JForStatement) { + // Cannot add decl statements to a for statement increments list. + JForStatement forStmt = (JForStatement) x; + banList.addAll(forStmt.getIncrements()); + } return super.visit(x, ctx); }
diff --git a/dev/core/test/com/google/gwt/dev/jjs/impl/TempLocalVisitorTest.java b/dev/core/test/com/google/gwt/dev/jjs/impl/TempLocalVisitorTest.java index d80cd71..e166c09 100644 --- a/dev/core/test/com/google/gwt/dev/jjs/impl/TempLocalVisitorTest.java +++ b/dev/core/test/com/google/gwt/dev/jjs/impl/TempLocalVisitorTest.java
@@ -18,14 +18,11 @@ import com.google.gwt.core.ext.UnableToCompleteException; import com.google.gwt.dev.jjs.SourceInfo; import com.google.gwt.dev.jjs.ast.Context; -import com.google.gwt.dev.jjs.ast.JBinaryOperation; -import com.google.gwt.dev.jjs.ast.JDeclarationStatement; import com.google.gwt.dev.jjs.ast.JExpression; +import com.google.gwt.dev.jjs.ast.JExpressionStatement; import com.google.gwt.dev.jjs.ast.JLocal; import com.google.gwt.dev.jjs.ast.JLocalRef; import com.google.gwt.dev.jjs.ast.JMethod; -import com.google.gwt.dev.jjs.ast.JPostfixOperation; -import com.google.gwt.dev.jjs.ast.JPrefixOperation; import com.google.gwt.dev.jjs.ast.JProgram; import com.google.gwt.dev.jjs.ast.JType; @@ -34,54 +31,31 @@ */ public class TempLocalVisitorTest extends JJSTestBase { /** - * For testing purposes; replaces all non-lvalue expressions with a temp. + * NOTE: for testing purposes only! This transform does not even try to + * preserve language semantics, such as evaluation order. It's purely meant as + * an exercise for TempLocalVisitor. */ private static final class AlwaysReplacer extends TempLocalVisitor { + /** + * Don't bother replacing entire JExpressionStatements with a temp. + */ + private JExpression dontBother; + + @Override + public boolean visit(JExpressionStatement x, Context ctx) { + dontBother = x.getExpr(); + return super.visit(x, ctx); + } + @Override public void endVisit(JExpression x, Context ctx) { - SourceInfo info = x.getSourceInfo(); - JType type = x.getType(); - JLocal local = createTempLocal(info, type); - local.getDeclarationStatement().initializer = x; - ctx.replaceMe(new JLocalRef(info, local)); - } - - @Override - public boolean visit(JBinaryOperation x, Context ctx) { - super.visit(x, ctx); - if (x.getRhs() != null) { - JExpression newRhs = accept(x.getRhs()); - if (newRhs != x.getRhs()) { - ctx.replaceMe(new JBinaryOperation(x.getSourceInfo(), x.getType(), - x.getOp(), x.getLhs(), newRhs)); - } + if (x != dontBother && !ctx.isLvalue()) { + SourceInfo info = x.getSourceInfo(); + JType type = x.getType(); + JLocal local = createTempLocal(info, type); + local.getDeclarationStatement().initializer = x; + ctx.replaceMe(new JLocalRef(info, local)); } - return false; - } - - @Override - public boolean visit(JDeclarationStatement x, Context ctx) { - super.visit(x, ctx); - if (x.initializer != null) { - x.initializer = accept(x.initializer); - } - return false; - } - - @Override - public boolean visit(JPostfixOperation x, Context ctx) { - if (x.getOp().isModifying()) { - return false; - } - return true; - } - - @Override - public boolean visit(JPrefixOperation x, Context ctx) { - if (x.getOp().isModifying()) { - return false; - } - return true; } } @@ -114,15 +88,16 @@ public void testForStatement() throws Exception { StringBuilder original = new StringBuilder(); - original.append("for (int i = 3; true; );"); + original.append("for (int i = 0; true; i += 1);"); StringBuilder expected = new StringBuilder(); /* - * TODO(scottb): technically $t1 could be part of the for statement's - * initializer list. + * TODO(scottb): technically $t1 and $t2 could be part of the for + * statement's initializer list. */ expected.append("boolean $t1 = true;"); - expected.append("for (int $t0 = 3, i = $t0; $t1; );"); + expected.append("int $t2 = 1;"); + expected.append("for (int $t0 = 0, i = $t0; $t1; i += $t2);"); assertTransform(original.toString()).into(expected.toString()); }