Fix https://github.com/julianhyde/linq4j/issues/27, "Incorrectly inlines non-final variable".
diff --git a/src/main/java/net/hydromatic/linq4j/Linq4j.java b/src/main/java/net/hydromatic/linq4j/Linq4j.java
index b854f64..0b2ff67 100644
--- a/src/main/java/net/hydromatic/linq4j/Linq4j.java
+++ b/src/main/java/net/hydromatic/linq4j/Linq4j.java
@@ -34,7 +34,7 @@ private Linq4j() {}
private static final Method AUTO_CLOSEABLE_CLOSE_METHOD =
getMethod("java.lang.AutoCloseable", "close");
- private static Method getMethod(String className, String methodName,
+ public static Method getMethod(String className, String methodName,
Class... parameterTypes) {
try {
return Class.forName(className).getMethod(methodName, parameterTypes);
diff --git a/src/main/java/net/hydromatic/linq4j/expressions/BlockBuilder.java b/src/main/java/net/hydromatic/linq4j/expressions/BlockBuilder.java
index 207705a..2170594 100644
--- a/src/main/java/net/hydromatic/linq4j/expressions/BlockBuilder.java
+++ b/src/main/java/net/hydromatic/linq4j/expressions/BlockBuilder.java
@@ -231,11 +231,10 @@ protected boolean isSimpleExpression(Expression expr) {
}
protected boolean isSafeForReuse(DeclarationStatement decl) {
- return (decl.modifiers & Modifier.FINAL) != 0
- && decl.initializer != null;
+ return (decl.modifiers & Modifier.FINAL) != 0;
}
- protected void addExpresisonForReuse(DeclarationStatement decl) {
+ protected void addExpressionForReuse(DeclarationStatement decl) {
if (isSafeForReuse(decl)) {
Expression expr = normalizeDeclaration(decl);
expressionForReuse.put(expr, decl);
@@ -282,7 +281,7 @@ public void add(Statement statement) {
if (!variables.add(name)) {
throw new AssertionError("duplicate variable " + name);
}
- addExpresisonForReuse(decl);
+ addExpressionForReuse(decl);
}
}
@@ -343,11 +342,15 @@ private boolean optimize(Visitor optimizer, boolean performInline) {
DeclarationStatement statement = (DeclarationStatement) oldStatement;
final Slot slot = useCounter.map.get(statement.parameter);
int count = slot == null ? Integer.MAX_VALUE - 10 : slot.count;
- if (count > 1 && isSafeForReuse(statement)
- && isSimpleExpression(statement.initializer)) {
+ if (count > 1 && isSimpleExpression(statement.initializer)) {
// Inline simple final constants
count = 1;
}
+ if (!isSafeForReuse(statement)) {
+ // Don't inline variables that are not final. They might be assigned
+ // more than once.
+ count = 100;
+ }
if (statement.parameter.name.startsWith("_")) {
// Don't inline variables whose name begins with "_". This
// is a hacky way to prevent inlining. E.g.
@@ -397,7 +400,7 @@ && isSimpleExpression(
}
if (oldStatement != OptimizeVisitor.EMPTY_STATEMENT) {
if (oldStatement instanceof DeclarationStatement) {
- addExpresisonForReuse((DeclarationStatement) oldStatement);
+ addExpressionForReuse((DeclarationStatement) oldStatement);
}
statements.add(oldStatement);
}
diff --git a/src/test/java/net/hydromatic/linq4j/test/InlinerTest.java b/src/test/java/net/hydromatic/linq4j/test/InlinerTest.java
index 185e2c1..17af215 100644
--- a/src/test/java/net/hydromatic/linq4j/test/InlinerTest.java
+++ b/src/test/java/net/hydromatic/linq4j/test/InlinerTest.java
@@ -19,12 +19,16 @@
import net.hydromatic.linq4j.expressions.*;
+import org.hamcrest.CoreMatchers;
import org.junit.Before;
import org.junit.Test;
+import java.lang.reflect.Modifier;
+
import static net.hydromatic.linq4j.test.BlockBuilderBase.*;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
/**
* Tests expression inlining in BlockBuilder.
@@ -106,12 +110,28 @@ public void prepareBuilder() {
}
@Test public void testAssignInConditionOptimizedOut() {
+ checkAssignInConditionOptimizedOut(Modifier.FINAL,
+ "{\n"
+ + " return 1 != a ? b : c;\n"
+ + "}\n");
+ }
+
+ @Test public void testAssignInConditionNotOptimizedWithoutFinal() {
+ checkAssignInConditionOptimizedOut(0,
+ "{\n"
+ + " int t;\n"
+ + " return (t = 1) != a ? b : c;\n"
+ + "}\n");
+ }
+
+ void checkAssignInConditionOptimizedOut(int modifiers, String s) {
// int t;
// return (t = 1) != a ? b : c
final BlockBuilder builder = new BlockBuilder(true);
- final ParameterExpression t = Expressions.parameter(int.class, "t");
+ final ParameterExpression t =
+ Expressions.parameter(int.class, "t");
- builder.add(Expressions.declare(0, t, null));
+ builder.add(Expressions.declare(modifiers, t, null));
Expression v = builder.append("v",
Expressions.makeTernary(ExpressionType.Conditional,
@@ -121,11 +141,8 @@ public void prepareBuilder() {
Expressions.parameter(int.class, "b"),
Expressions.parameter(int.class, "c")));
builder.add(Expressions.return_(null, v));
- assertEquals(
- "{\n"
- + " return 1 != a ? b : c;\n"
- + "}\n",
- Expressions.toString(builder.toBlock()));
+ assertThat(Expressions.toString(builder.toBlock()),
+ CoreMatchers.equalTo(s));
}
@Test public void testAssignInConditionMultipleUsageNonOptimized() {
diff --git a/src/test/java/net/hydromatic/linq4j/test/OptimizerTest.java b/src/test/java/net/hydromatic/linq4j/test/OptimizerTest.java
index 8ea31c1..b31e996 100644
--- a/src/test/java/net/hydromatic/linq4j/test/OptimizerTest.java
+++ b/src/test/java/net/hydromatic/linq4j/test/OptimizerTest.java
@@ -17,12 +17,17 @@
*/
package net.hydromatic.linq4j.test;
+import net.hydromatic.linq4j.Linq4j;
import net.hydromatic.linq4j.expressions.*;
import org.junit.Test;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+
import static net.hydromatic.linq4j.test.BlockBuilderBase.*;
+import static org.hamcrest.CoreMatchers.equalTo;
import static org.junit.Assert.*;
/**
@@ -659,6 +664,78 @@ public class OptimizerTest {
assertEquals("{\n return false;\n}\n",
optimize(Expressions.call(Boolean.class, "valueOf", FALSE)));
}
+
+ @Test public void testAssign() {
+ // long x = 0;
+ // final long y = System.currentTimeMillis();
+ // if (System.nanoTime() > 0) {
+ // x = y;
+ // }
+ // System.out.println(x);
+ //
+ // In bug https://github.com/julianhyde/linq4j/issues/27, this was
+ // incorrectly optimized to
+ //
+ // if (System.nanoTime() > 0L) {
+ // System.currentTimeMillis();
+ // }
+ // System.out.println(0L);
+ final ParameterExpression x_ = Expressions.parameter(long.class, "x");
+ final ParameterExpression y_ = Expressions.parameter(long.class, "y");
+ final Method mT = Linq4j.getMethod("java.lang.System", "currentTimeMillis");
+ final Method mNano = Linq4j.getMethod("java.lang.System", "nanoTime");
+ final ConstantExpression zero = Expressions.constant(0L);
+ assertThat(
+ optimize(
+ Expressions.block(
+ Expressions.declare(0, x_, zero),
+ Expressions.declare(Modifier.FINAL, y_, Expressions.call(mT)),
+ Expressions.ifThen(
+ Expressions.greaterThan(Expressions.call(mNano), zero),
+ Expressions.statement(Expressions.assign(x_, y_))),
+ Expressions.statement(
+ Expressions.call(
+ Expressions.field(null, System.class, "out"),
+ "println",
+ x_)))),
+ equalTo(
+ "{\n"
+ + " long x = 0L;\n"
+ + " if (System.nanoTime() > 0L) {\n"
+ + " x = System.currentTimeMillis();\n"
+ + " }\n"
+ + " System.out.println(x);\n"
+ + "}\n"));
+ }
+
+ @Test public void testAssign2() {
+ // long x = 0;
+ // final long y = System.currentTimeMillis();
+ // if (System.currentTimeMillis() > 0) {
+ // x = y;
+ // }
+ //
+ // Make sure we don't fold two calls to System.currentTimeMillis into one.
+ final ParameterExpression x_ = Expressions.parameter(long.class, "x");
+ final ParameterExpression y_ = Expressions.parameter(long.class, "y");
+ final Method mT = Linq4j.getMethod("java.lang.System", "currentTimeMillis");
+ final ConstantExpression zero = Expressions.constant(0L);
+ assertThat(
+ optimize(
+ Expressions.block(
+ Expressions.declare(0, x_, zero),
+ Expressions.declare(Modifier.FINAL, y_, Expressions.call(mT)),
+ Expressions.ifThen(
+ Expressions.greaterThan(Expressions.call(mT), zero),
+ Expressions.statement(Expressions.assign(x_, y_))))),
+ equalTo(
+ "{\n"
+ + " long x = 0L;\n"
+ + " if (System.currentTimeMillis() > 0L) {\n"
+ + " x = System.currentTimeMillis();\n"
+ + " }\n"
+ + "}\n"));
+ }
}
// End OptimizerTest.java
