Reintroduces JsInliner patch (3rd time is a charm?). Fix for issue 5707. With minor tweaks to Ray Cromwell's original version of this patch. Review at http://gwt-code-reviews.appspot.com/1451802 Review by: cromwellian@google.com git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@10248 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/dev/core/src/com/google/gwt/dev/js/JsInliner.java b/dev/core/src/com/google/gwt/dev/js/JsInliner.java index caf83ea..00c2e0a 100644 --- a/dev/core/src/com/google/gwt/dev/js/JsInliner.java +++ b/dev/core/src/com/google/gwt/dev/js/JsInliner.java
@@ -612,10 +612,25 @@ private boolean maintainsOrder = true; private final List<JsName> toEvaluate; private final List<JsName> unevaluated; + private final Set<JsName> paramsOrLocals = new HashSet<JsName>(); - public EvaluationOrderVisitor(List<JsName> toEvaluate) { + 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 ctx) { + paramsOrLocals.add(x.getName()); + } + + @Override + public boolean visit(JsVar x, JsContext ctx) { + // record this before visiting initializer + paramsOrLocals.add(x.getName()); + return true; + } + }.accept(callee); } @Override @@ -670,12 +685,7 @@ */ @Override public void endVisit(JsInvocation x, JsContext 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; } } @@ -709,11 +719,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 violation. + * + * A reference to a volatile parameter is ok if it 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; } @@ -1614,7 +1648,7 @@ * code to be inlined, but at a cost of larger JS output. */ private static final double MAX_COMPLEXITY_INCREASE = Double.parseDouble(System.getProperty( - "gwt.jsinlinerRatio", "5.0")); + "gwt.jsinlinerRatio", "1.7")); /** * Static entry point used by JavaToJavaScriptCompiler. @@ -1934,7 +1968,7 @@ * order. */ EvaluationOrderVisitor orderVisitor = new EvaluationOrderVisitor( - requiredOrder); + requiredOrder, callee); orderVisitor.accept(toInline); if (!orderVisitor.maintainsOrder()) { return false;
diff --git a/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java b/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java index fdecc01..9bdae0f 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java
@@ -85,6 +85,120 @@ } /** + * Test that a global array reference breaks argument ordering. + */ + public void testOrderingArrayGlobal() throws Exception { + StringBuffer code = new StringBuffer(); + + code.append("var array; "); + code.append("function clinit() { clinit = null; }"); + + // callee references array[0] before evaluating argument + code.append("function callee(arg) { 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 local reference does not break argument ordering. + */ + public void testOrderingArrayLocal() 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();"); + + StringBuffer expected = new StringBuffer(); + expected.append("function clinit() { clinit = null; }"); + expected.append("function caller() { var array; array[0] + (clinit(), 2); }"); + expected.append("caller();"); + + compare(expected.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. */ public void testOrderingNew() throws Exception {