Fixes a problem where inlining an JS method with no return statement fails to evaluate to "undefined". Review by: bobv (postmortem) git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@1558 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 801a964..0f94ec2 100644 --- a/dev/core/src/com/google/gwt/dev/js/JsInliner.java +++ b/dev/core/src/com/google/gwt/dev/js/JsInliner.java
@@ -577,8 +577,8 @@ * configured to collect idents from qualified xor unqualified JsNameRefs. */ private static class IdentCollector extends JsVisitor { - private final Set<String> idents = new HashSet<String>(); private final boolean collectQualified; + private final Set<String> idents = new HashSet<String>(); public IdentCollector(boolean collectQualified) { this.collectQualified = collectQualified; @@ -610,8 +610,8 @@ * statements if the context of the invocation would allow this. */ private static class InliningVisitor extends JsModVisitor { - private final Stack<JsFunction> functionStack = new Stack<JsFunction>(); private final Set<JsFunction> blacklist = new HashSet<JsFunction>(); + private final Stack<JsFunction> functionStack = new Stack<JsFunction>(); private final JsProgram program; public InliningVisitor(JsProgram program) { @@ -737,6 +737,7 @@ List<JsExpression> hoisted = new ArrayList<JsExpression>( statements.size()); + boolean sawReturnStatement = false; for (JsStatement statement : statements) { /* * Create replacement expressions to use in place of the original @@ -753,41 +754,37 @@ hoisted.add(h); - if (hoistedExpressionTerminal(statement)) { + if (isReturnStatement(statement)) { + sawReturnStatement = true; break; } } /* - * Determine the expressions that will be used to construct the - * replacement expression. + * If the inlined method has no return statement, synthesize an undefined + * reference. It will be reclaimed if the method call is from a + * JsExprStmt. */ - JsExpression op; + if (!sawReturnStatement) { + hoisted.add(program.getUndefinedLiteral()); + } - if (hoisted.size() == 0) { - /* - * There are no expressions evaluated in the target function, so we'll - * try to replace the invocation with nothing. The null literal may be - * reclaimed during the JsExprStmt cleanup pass. - */ - op = program.getNullLiteral(); + assert (hoisted.size() > 0); - } else { - /* - * Build up the new comma expression from right-to-left; building the - * rightmost comma expressions first. Bootstrapping with i.previous() - * ensures that this logic will function correctly in the case of a - * single expression. - */ - ListIterator<JsExpression> i = hoisted.listIterator(hoisted.size()); - op = i.previous(); - while (i.hasPrevious()) { - JsBinaryOperation outerOp = new JsBinaryOperation( - JsBinaryOperator.COMMA); - outerOp.setArg1(i.previous()); - outerOp.setArg2(op); - op = outerOp; - } + /* + * Build up the new comma expression from right-to-left; building the + * rightmost comma expressions first. Bootstrapping with i.previous() + * ensures that this logic will function correctly in the case of a single + * expression. + */ + ListIterator<JsExpression> i = hoisted.listIterator(hoisted.size()); + JsExpression op = i.previous(); + while (i.hasPrevious()) { + JsBinaryOperation outerOp = new JsBinaryOperation( + JsBinaryOperator.COMMA); + outerOp.setArg1(i.previous()); + outerOp.setArg2(op); + op = outerOp; } // Confirm that the expression conforms to the desired heuristics @@ -890,8 +887,8 @@ * Generally speaking, we disallow the use of parameters as lvalues. */ private static class ParameterUsageVisitor extends JsVisitor { - private final Set<JsName> parameterNames; private boolean lvalue = false; + private final Set<JsName> parameterNames; public ParameterUsageVisitor(Set<JsName> parameterNames) { this.parameterNames = parameterNames; @@ -1008,8 +1005,8 @@ * the lifetime of the program. */ private static class RedefinedFunctionCollector extends JsVisitor { - private final Set<JsFunction> redefined = new HashSet<JsFunction>(); private final Map<JsName, JsFunction> nameMap = new IdentityHashMap<JsName, JsFunction>(); + private final Set<JsFunction> redefined = new HashSet<JsFunction>(); /** * Look for assignments to JsNames whose static references are JsFunctions. @@ -1157,8 +1154,8 @@ * identifiers to resolve to different values. */ private static class StableNameChecker extends JsVisitor { - private final JsScope callerScope; private final JsScope calleeScope; + private final JsScope callerScope; private final Collection<JsName> parameterNames; private boolean stable = true; @@ -1205,15 +1202,6 @@ } /** - * When attempting to inline an invocation, this constant determines the - * maximum allowable ratio of potential inlined complexity to initial - * complexity. This acts as a brake on very large expansions from bloating the - * the generated output. Increasing this number will allow larger sections of - * code to be inlined, but at a cost of larger JS output. - */ - private static final int MAX_COMPLEXITY_INCREASE = 10; - - /** * A List of expression types that are known to never be affected by * side-effects. Used by {@link #alwaysFlexible(JsExpression)}. */ @@ -1223,6 +1211,15 @@ JsThisRef.class}); /** + * When attempting to inline an invocation, this constant determines the + * maximum allowable ratio of potential inlined complexity to initial + * complexity. This acts as a brake on very large expansions from bloating the + * the generated output. Increasing this number will allow larger sections of + * code to be inlined, but at a cost of larger JS output. + */ + private static final int MAX_COMPLEXITY_INCREASE = 5; + + /** * Static entry point used by JavaToJavaScriptCompiler. */ public static boolean exec(JsProgram program) { @@ -1328,13 +1325,10 @@ if (statement instanceof JsExprStmt) { JsExprStmt exprStmt = (JsExprStmt) statement; expression = exprStmt.getExpression(); - } else if (statement instanceof JsReturn) { JsReturn ret = (JsReturn) statement; expression = ret.getExpr(); - } else { - // TODO If additional statements are supported, update hETerminal() return null; } @@ -1342,16 +1336,6 @@ } /** - * This is used in combination with {@link #hoistedExpression(JsStatement)} to - * indicate if a given statement would terminate the list of hoisted - * expressions. - */ - private static boolean hoistedExpressionTerminal(JsStatement statement) { - // TODO keep this in synch with hoistedExpression - return statement instanceof JsReturn; - } - - /** * Given a JsInvocation, determine if it is invoking a JsFunction that is * specified to be executed only once during the program's lifetime. */ @@ -1475,6 +1459,15 @@ } /** + * This is used in combination with {@link #hoistedExpression(JsStatement)} to + * indicate if a given statement would terminate the list of hoisted + * expressions. + */ + private static boolean isReturnStatement(JsStatement statement) { + return statement instanceof JsReturn; + } + + /** * Utility class. */ private JsInliner() {
diff --git a/dev/core/src/com/google/gwt/dev/js/ast/JsProgram.java b/dev/core/src/com/google/gwt/dev/js/ast/JsProgram.java index a8ca1bf..9c222b1 100644 --- a/dev/core/src/com/google/gwt/dev/js/ast/JsProgram.java +++ b/dev/core/src/com/google/gwt/dev/js/ast/JsProgram.java
@@ -139,6 +139,10 @@ return trueLiteral; } + public JsNameRef getUndefinedLiteral() { + return rootScope.findExistingName("undefined").makeRef(); + } + public void traverse(JsVisitor v, JsContext<JsProgram> ctx) { if (v.visit(this, ctx)) { v.accept(globalBlock);