Revision: 9362
Author: [email protected]
Date: Mon Dec 6 14:51:53 2010
Log: Partial fix for
http://code.google.com/p/google-web-toolkit/issues/detail?id=5707
Modify JsInliner to avoid propagating expressions incorrectly.
in EvaluationOrderVisitor, two changes were made. First of all, a clinit()
call
is treated as violating order, the same as any other invocation.
Secondly, a JsNameRef that can be affected by side effects (a global,
non-param,
non-local) that is visited when there are still expressions that need
evaluation
first, is treated as violating order.
This fixes the showcase issue, but is still insufficient. Side-effect
inducing
expressions (assignment, post/prefix increment ops) are still not handled.
The
exemption of JsNameRefs that are locals is also insufficient, since a side
effect inducing parameter value may be aliased to the same object as this
local.
After your review, I may remove it and just ban any non-param reference from
occuring while there are still params to be evaluated.
I tested this and it seems to fix the problem.
Review by: [email protected]
http://code.google.com/p/google-web-toolkit/source/detail?r=9362
Modified:
/trunk/dev/core/src/com/google/gwt/dev/js/JsInliner.java
/trunk/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java
=======================================
--- /trunk/dev/core/src/com/google/gwt/dev/js/JsInliner.java Mon Sep 20
06:50:17 2010
+++ /trunk/dev/core/src/com/google/gwt/dev/js/JsInliner.java Mon Dec 6
14:51:53 2010
@@ -611,12 +611,37 @@
private boolean maintainsOrder = true;
private final List<JsName> toEvaluate;
private final List<JsName> unevaluated;
-
- public EvaluationOrderVisitor(List<JsName> toEvaluate) {
+ private final Set<JsName> paramsOrLocals = new HashSet<JsName>();
+
+ public EvaluationOrderVisitor(List<JsName> toEvaluate, JsFunction
callee) {
this.toEvaluate = toEvaluate;
this.unevaluated = new ArrayList<JsName>(toEvaluate);
+ // collect params and locals from callee function
+ new JsVisitor() {
+ @Override
+ public void endVisit(JsParameter x, JsContext<JsParameter> ctx) {
+ paramsOrLocals.add(x.getName());
+ }
+
+ @Override
+ public boolean visit(JsVar x, JsContext<JsVar> ctx) {
+ // record this before visiting initializer
+ paramsOrLocals.add(x.getName());
+ return true;
+ }
+ }.accept(callee);
}
+ /**
+ * Referencing an array breaks order if there are unevaluated
arguments.
+ */
+ @Override
+ public void endVisit(JsArrayAccess x, JsContext<JsExpression> ctx) {
+ if (unevaluated.size() > 0) {
+ maintainsOrder = false;
+ }
+ }
+
@Override
public void endVisit(JsBinaryOperation x, JsContext<JsExpression> ctx)
{
JsBinaryOperator op = x.getOperator();
@@ -669,12 +694,7 @@
*/
@Override
public void endVisit(JsInvocation x, JsContext<JsExpression> ctx) {
- /*
- * The check for isExecuteOnce() is potentially incorrect here,
however
- * the original Java semantics of the clinit would have made the code
- * incorrect anyway.
- */
- if ((isExecuteOnce(x) == null) && unevaluated.size() > 0) {
+ if (unevaluated.size() > 0) {
maintainsOrder = false;
}
}
@@ -710,11 +730,35 @@
return maintainsOrder && unevaluated.size() == 0;
}
+ /**
+ * Check to see if the evaluation of this JsName will break program
order assumptions given
+ * the parameters left to be substituted.
+ *
+ * The cases are as follows:
+ * 1) JsName is a function parameter name which has side effects or is
affected by side effects
+ * (hereafter called 'volatile'), so it will be in 'toEvaluate'
+ * 2) JsName is a function parameter which is not volatile (not in
toEvaluate)
+ * 3) JsName is a reference to a global variable
+ * 4) JsName is a reference to a local variable
+ *
+ * A reference to a global while there are still parameters left to
evaluate / substitute
+ * implies an order volation.
+ *
+ * A reference to a volatile parameter is ok if it if is the next
parameter in sequence to
+ * be evaluated (beginning of unevaluated list). Else, it is either
being evaluated out of
+ * order with respect to other parameters, or it is being evaluated
more than once.
+ */
private void checkName(JsName name) {
if (!toEvaluate.contains(name)) {
+ // if the name is a non-local/non-parameter (e.g. global) and
there are params left to eval
+ if (!paramsOrLocals.contains(name) && unevaluated.size() > 0) {
+ maintainsOrder = false;
+ }
+ // else this may be a local, or all volatile params have already
been evaluated, so it's ok.
return;
}
+ // either this param is being evaled twice, or out of order
if (unevaluated.size() == 0 || !unevaluated.remove(0).equals(name)) {
maintainsOrder = false;
}
@@ -1945,7 +1989,7 @@
* order.
*/
EvaluationOrderVisitor orderVisitor = new EvaluationOrderVisitor(
- requiredOrder);
+ requiredOrder, callee);
orderVisitor.accept(toInline);
if (!orderVisitor.maintainsOrder()) {
return false;
=======================================
--- /trunk/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java Tue Apr
13 06:08:53 2010
+++ /trunk/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java Mon Dec
6 14:51:53 2010
@@ -79,6 +79,94 @@
+ "function c1() { return ex2 ? a1() :c1(); } c1()";
compare(expected, input);
}
+
+ /**
+ * Test that an array reference breaks argument ordering.
+ */
+ public void testOrderingArray() throws Exception {
+ StringBuffer code = new StringBuffer();
+
+ code.append("function clinit() { clinit = null; }");
+
+ // callee references array[0] before evaluating argument
+ code.append("function callee(arg) { var array; return array[0] + arg;
}");
+
+ // caller invokes callee with a multi that runs clinit()
+ code.append("function caller() { callee((clinit(),2)); }");
+
+ // bootstrap the program
+ code.append("caller();");
+
+ compare(code.toString(), code.toString());
+ }
+
+ /**
+ * Test that a field reference breaks argument ordering.
+ */
+ public void testOrderingField() throws Exception {
+ StringBuffer code = new StringBuffer();
+
+ code.append("function clinit() { clinit = null; }");
+
+ // callee references field.x before evaluating argument
+ code.append("function callee(arg) { var field; return field.x + arg;
}");
+
+ // caller invokes callee with a multi that runs clinit()
+ code.append("function caller() { callee((clinit(),2)); }");
+
+ // bootstrap the program
+ code.append("caller();");
+
+ compare(code.toString(), code.toString());
+ }
+
+ /**
+ * Test that a global variable breaks argument ordering.
+ */
+ public void testOrderingGlobal() throws Exception {
+ StringBuffer code = new StringBuffer();
+ // A global variable x
+ code.append("var x;");
+
+ // clinit() sets up x
+ code.append("function clinit() { x = 1; clinit = null; }");
+
+ // callee references x before evaluating argument
+ code.append("function callee(arg) { alert(x); return arg; }");
+
+ // caller invokes callee with a multi that runs clinit()
+ code.append("function caller() { callee((clinit(),2)); }");
+
+ // bootstrap the program
+ code.append("caller();");
+
+ compare(code.toString(), code.toString());
+ }
+
+ /**
+ * Test that a local variable does not break argument ordering.
+ */
+ public void testOrderingLocal() throws Exception {
+ StringBuffer code = new StringBuffer();
+
+ code.append("function clinit() { clinit = null; }");
+
+ // callee references y before evaluating argument
+ code.append("function callee(arg) { var y; y=2; return arg; }");
+
+ // caller invokes callee with a multi that runs clinit()
+ code.append("function caller() { return callee((clinit(),3)); }");
+
+ // bootstrap the program
+ code.append("caller();");
+
+ StringBuffer expected = new StringBuffer();
+
+ expected.append("function clinit() { clinit = null; }");
+ expected.append("function caller() {var y; return y=2,clinit(),3;}");
+ expected.append("caller();");
+ compare(expected.toString(), code.toString());
+ }
/**
* Test that a new expression breaks argument ordering.
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
