diff --git a/src/com/google/javascript/jscomp/MinimizedCondition.java b/src/com/google/javascript/jscomp/MinimizedCondition.java index 36233b51112..32c21f5da97 100644 --- a/src/com/google/javascript/jscomp/MinimizedCondition.java +++ b/src/com/google/javascript/jscomp/MinimizedCondition.java @@ -16,13 +16,10 @@ package com.google.javascript.jscomp; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableList; -import com.google.javascript.rhino.IR; import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Token; -import java.util.Collections; -import java.util.Comparator; /** * A class that represents a minimized conditional expression. @@ -50,42 +47,28 @@ enum MinimizationStyle { /** A representation equivalent to the original condition. */ private final MeasuredNode positive; + /** A representation equivalent to the negation of the original condition. */ private final MeasuredNode negative; - /** A placeholder at the same AST location as the original condition */ - private Node placeholder; - private MinimizedCondition(MeasuredNode p, MeasuredNode n) { - Preconditions.checkArgument(p.node.getParent() == null); - Preconditions.checkArgument(n.node.getParent() == null); positive = p; negative = n.change(); } - Node getPlaceholder() { - return placeholder; - } - - MinimizedCondition setPlaceholder(Node placeholder) { - this.placeholder = placeholder; - return this; - } - /** - * Remove the passed condition node from the AST, and then return a - * MinimizedCondition that represents the condition node after + * Returns a MinimizedCondition that represents the condition node after * minimization. */ static MinimizedCondition fromConditionNode(Node n) { + Preconditions.checkState(n.getParent() != null); switch (n.getToken()) { case NOT: case AND: case OR: case HOOK: case COMMA: - Node placeholder = swapWithPlaceholderNode(n); - return computeMinimizedCondition(n).setPlaceholder(placeholder); + return computeMinimizedCondition(n); default: return unoptimized(n); } @@ -124,181 +107,177 @@ MeasuredNode getMinimized(MinimizationStyle style) { * Return a MeasuredNode of the given condition node, without minimizing * the result. *

- * Since a MinimizedCondition necessarily must contain two trees, this - * method sets the negative side to a {@link Token#SCRIPT} node (never valid - * inside an expression) with an unreasonably high length so that it will + * Since a MinimizedCondition necessarily must contain two trees, + * this method sets the negative side to a invalid node + * with an unreasonably high length so that it will * never be chosen by {@link #getMinimized}. * - * @param n the conditional expression tree to minimize. - * This must be connected to the AST, and will be swapped - * with a placeholder node during minimization. + * @param n the conditional expression tree * @return a MinimizedCondition object representing that tree. */ static MinimizedCondition unoptimized(Node n) { Preconditions.checkNotNull(n.getParent()); - Node placeholder = swapWithPlaceholderNode(n); - MeasuredNode pos = new MeasuredNode(n, 0, false); - MeasuredNode neg = new MeasuredNode(IR.script(), Integer.MAX_VALUE, true); - return new MinimizedCondition(pos, neg).setPlaceholder(placeholder); + MeasuredNode pos = new MeasuredNode(n, null, 0, false); + MeasuredNode neg = new MeasuredNode(null, null, Integer.MAX_VALUE, true); + return new MinimizedCondition(pos, neg); } - /** - * Remove the given node from the AST, and replace it with a placeholder - * SCRIPT node. - * @return the new placeholder node. - */ - private static Node swapWithPlaceholderNode(Node n) { - Preconditions.checkNotNull(n.getParent()); - Node placeholder = IR.script(); - n.replaceWith(placeholder); - return placeholder; + /** return the best, prefer unchanged */ + static MeasuredNode pickBest(MeasuredNode a, MeasuredNode b) { + if (a.length == b.length) { + return (b.isChanged()) ? a : b; + } + + return (a.length < b.length) ? a : b; } /** * Minimize the condition at the given node. * * @param n the conditional expression tree to minimize. - * This must be connected to the AST, and will be swapped - * with a placeholder node during minimization. * @return a MinimizedCondition object representing that tree. */ private static MinimizedCondition computeMinimizedCondition(Node n) { - Preconditions.checkArgument(n.getParent() == null); switch (n.getToken()) { case NOT: { - MinimizedCondition subtree = - computeMinimizedCondition(n.getFirstChild().detach()); - ImmutableList positiveAsts = ImmutableList.of( - subtree.positive.cloneTree().addNot(), - subtree.negative.cloneTree()); - ImmutableList negativeAsts = ImmutableList.of( + MinimizedCondition subtree = computeMinimizedCondition(n.getFirstChild()); + MeasuredNode positive = pickBest( + MeasuredNode.addNode(n, subtree.positive), + subtree.negative); + MeasuredNode negative = pickBest( subtree.negative.negate(), subtree.positive); - return new MinimizedCondition( - Collections.min(positiveAsts, AST_LENGTH_COMPARATOR), - Collections.min(negativeAsts, AST_LENGTH_COMPARATOR)); + return new MinimizedCondition(positive, negative); } case AND: case OR: { - Token opType = n.getToken(); - Token complementType = opType == Token.AND ? Token.OR : Token.AND; - MinimizedCondition leftSubtree = - computeMinimizedCondition(n.getFirstChild().detach()); - MinimizedCondition rightSubtree = - computeMinimizedCondition(n.getLastChild().detach()); - ImmutableList positiveAsts = ImmutableList.of( - MeasuredNode.addNode(new Node(opType).srcref(n), - leftSubtree.positive.cloneTree(), - rightSubtree.positive.cloneTree()), - MeasuredNode.addNode(new Node(complementType).srcref(n), - leftSubtree.negative.cloneTree(), - rightSubtree.negative.cloneTree()).negate()); - ImmutableList negativeAsts = ImmutableList.of( - MeasuredNode.addNode(new Node(opType).srcref(n), + Node complementNode = new Node(n.getToken() == Token.AND ? Token.OR : Token.AND).srcref(n); + MinimizedCondition leftSubtree = computeMinimizedCondition(n.getFirstChild()); + MinimizedCondition rightSubtree = computeMinimizedCondition(n.getLastChild()); + MeasuredNode positive = pickBest( + MeasuredNode.addNode(n, + leftSubtree.positive, + rightSubtree.positive), + MeasuredNode.addNode(complementNode, + leftSubtree.negative, + rightSubtree.negative).negate()); + MeasuredNode negative = pickBest( + MeasuredNode.addNode(n, leftSubtree.positive, rightSubtree.positive).negate(), - MeasuredNode.addNode(new Node(complementType).srcref(n), + MeasuredNode.addNode(complementNode, leftSubtree.negative, - rightSubtree.negative)); - return new MinimizedCondition( - Collections.min(positiveAsts, AST_LENGTH_COMPARATOR), - Collections.min(negativeAsts, AST_LENGTH_COMPARATOR)); + rightSubtree.negative).change()); + return new MinimizedCondition(positive, negative); } case HOOK: { Node cond = n.getFirstChild(); Node thenNode = cond.getNext(); Node elseNode = thenNode.getNext(); - MinimizedCondition thenSubtree = - computeMinimizedCondition(thenNode.detach()); - MinimizedCondition elseSubtree = - computeMinimizedCondition(elseNode.detach()); - MeasuredNode posTree = MeasuredNode.addNode( - new Node(Token.HOOK, cond.cloneTree()).srcref(n), + MinimizedCondition thenSubtree = computeMinimizedCondition(thenNode); + MinimizedCondition elseSubtree = computeMinimizedCondition(elseNode); + MeasuredNode positive = MeasuredNode.addNode( + n, + MeasuredNode.forNode(cond), thenSubtree.positive, elseSubtree.positive); - MeasuredNode negTree = MeasuredNode.addNode( - new Node(Token.HOOK, cond.cloneTree()).srcref(n), + MeasuredNode negative = MeasuredNode.addNode( + n, + MeasuredNode.forNode(cond), thenSubtree.negative, elseSubtree.negative); - return new MinimizedCondition(posTree, negTree); + return new MinimizedCondition(positive, negative); } case COMMA: { Node lhs = n.getFirstChild(); - MinimizedCondition rhsSubtree = - computeMinimizedCondition(lhs.getNext().detach()); - MeasuredNode posTree = MeasuredNode.addNode( - new Node(Token.COMMA, lhs.cloneTree()).srcref(n), + MinimizedCondition rhsSubtree = computeMinimizedCondition(lhs.getNext()); + MeasuredNode positive = MeasuredNode.addNode( + n, + MeasuredNode.forNode(lhs), rhsSubtree.positive); - MeasuredNode negTree = MeasuredNode.addNode( - new Node(Token.COMMA, lhs.cloneTree()).srcref(n), + MeasuredNode negative = MeasuredNode.addNode( + n, + MeasuredNode.forNode(lhs), rhsSubtree.negative); - return new MinimizedCondition(posTree, negTree); + return new MinimizedCondition(positive, negative); } default: { - MeasuredNode pos = new MeasuredNode(n, 0, false); - MeasuredNode neg = pos.cloneTree().negate(); + MeasuredNode pos = MeasuredNode.forNode(n); + MeasuredNode neg = pos.negate(); return new MinimizedCondition(pos, neg); } } } - private static final Comparator AST_LENGTH_COMPARATOR = - new Comparator() { - @Override - public int compare(MeasuredNode o1, MeasuredNode o2) { - return o1.length - o2.length; - } - }; - /** An AST-node along with some additional metadata. */ static class MeasuredNode { - private Node node; - private int length; - private boolean changed; + private final Node node; + private final int length; + private final boolean changed; + private final MeasuredNode[] children; - Node getNode() { - return node; + MeasuredNode(Node n, MeasuredNode[] children, int len, boolean ch) { + this.node = n; + this.children = children; + this.length = len; + this.changed = ch; } boolean isChanged() { return changed; } - MeasuredNode(Node n, int len, boolean ch) { - node = n; - length = len; - changed = ch; + boolean isNot() { + return node.isNot(); + } + + MeasuredNode withoutNot() { + Preconditions.checkState(this.isNot()); + return (normalizeChildren(node, children)[0]).change(); } private MeasuredNode negate() { - this.change(); switch (node.getToken()) { case EQ: - node.setToken(Token.NE); - return this; + return updateToken(Token.NE); case NE: - node.setToken(Token.EQ); - return this; + return updateToken(Token.EQ); case SHEQ: - node.setToken(Token.SHNE); - return this; + return updateToken(Token.SHNE); case SHNE: - node.setToken(Token.SHEQ); - return this; + return updateToken(Token.SHEQ); + case NOT: + return withoutNot(); default: return this.addNot(); } } - private MeasuredNode change() { - this.changed = true; - return this; + static MeasuredNode[] normalizeChildren(Node node, MeasuredNode[] children) { + if (children != null || !node.hasChildren()) { + return children; + } else { + MeasuredNode[] measuredChildren = new MeasuredNode[node.getChildCount()]; + int child = 0; + for (Node c : node.children()) { + measuredChildren[child++] = forNode(c); + } + return measuredChildren; + } + } + + private MeasuredNode updateToken(Token token) { + return new MeasuredNode( + new Node(token).srcref(node), normalizeChildren(node, children), length, true); } private MeasuredNode addNot() { - node = new Node(Token.NOT, node).srcref(node); - length += estimateCostOneLevel(node); - return this; + return addNode( + new Node(Token.NOT).srcref(node), this).change(); + } + + private MeasuredNode change() { + return (isChanged()) ? this : new MeasuredNode(node, children, length, true); } /** @@ -312,37 +291,97 @@ private MeasuredNode addNot() { * @param n the node to be checked. * @return the number of negations and parentheses in the node. */ - private static int estimateCostOneLevel(Node n) { + private static int estimateCostOneLevel(Node n, MeasuredNode ...children) { int cost = 0; if (n.isNot()) { cost++; // A negation is needed. } int parentPrecedence = NodeUtil.precedence(n.getToken()); - for (Node child = n.getFirstChild(); - child != null; child = child.getNext()) { - if (PeepholeMinimizeConditions.isLowerPrecedence(child, parentPrecedence)) { + for (MeasuredNode child : children) { + if (child.isLowerPrecedenceThan(parentPrecedence)) { cost += 2; // A pair of parenthesis is needed. } } return cost; } - private MeasuredNode cloneTree() { - return new MeasuredNode(node.cloneTree(), length, changed); + /** + * Whether the node type has lower precedence than "precedence" + */ + boolean isLowerPrecedenceThan(int precedence) { + return NodeUtil.precedence(node.getToken()) < precedence; } - private static MeasuredNode addNode(Node parent, MeasuredNode... children) { + /** + * The returned MeasuredNode is only marked as changed if the children + * are marked as changed. + */ + private static MeasuredNode addNode(Node parent, MeasuredNode ...children) { int cost = 0; boolean changed = false; for (MeasuredNode child : children) { - parent.addChildToBack(child.node); cost += child.length; changed = changed || child.changed; } - cost += estimateCostOneLevel(parent); - return new MeasuredNode(parent, cost, changed); + cost += estimateCostOneLevel(parent, children); + return new MeasuredNode(parent, children, cost, changed); } - } + /** + * Return a MeasuredNode for a non-particapting AST Node. This is + * used for leaf expression nodes. + */ + private static MeasuredNode forNode(Node n) { + return new MeasuredNode(n, null, 0, false); + } + /** + * Whether the MeasuredNode is a change from the original. + * This can either be a change within the original AST tree or a + * replacement of the original node. + */ + public boolean willChange(Node original) { + Preconditions.checkNotNull(original); + return original != this.node || this.isChanged(); + } + + /** + * Update the AST for the result of this MeasuredNode. + * This can either be a change within the original AST tree or a + * replacement of the original node. + */ + public Node applyTo(Node original) { + Preconditions.checkNotNull(original); + Preconditions.checkState(willChange(original)); + Node replacement = buildReplacement(); + if (original != replacement) { + safeDetach(replacement); + original.replaceWith(replacement); + } + return replacement; + } + + /** Detach a node only IIF it is in the tree */ + private Node safeDetach(Node n) { + return (n.getParent() != null) ? n.detach() : n; + } + + /** + * Build the final AST structure, detaching component Nodes as necessary + * from the original AST. The root Node, if currently attached is left attached + * to avoid the need to keep track of its position. + */ + @VisibleForTesting + Node buildReplacement() { + if (children != null) { + node.detachChildren(); + for (MeasuredNode child : children) { + Node replacementChild = safeDetach(child.buildReplacement()); + node.addChildToBack(replacementChild); + } + } + return node; + } + + } } diff --git a/src/com/google/javascript/jscomp/PeepholeMinimizeConditions.java b/src/com/google/javascript/jscomp/PeepholeMinimizeConditions.java index ae0ea796153..bbc83a0e829 100644 --- a/src/com/google/javascript/jscomp/PeepholeMinimizeConditions.java +++ b/src/com/google/javascript/jscomp/PeepholeMinimizeConditions.java @@ -18,6 +18,7 @@ import com.google.common.base.Preconditions; import com.google.common.base.Predicate; +import com.google.javascript.jscomp.MinimizedCondition.MeasuredNode; import com.google.javascript.jscomp.MinimizedCondition.MinimizationStyle; import com.google.javascript.rhino.IR; import com.google.javascript.rhino.Node; @@ -203,8 +204,8 @@ && isReturnBlock(thenBranch) newCond.addChildToBack(nextCond); reportCodeChange(); } - } else if (nextNode != null && elseBranch == null && - isReturnBlock(thenBranch) && isReturnExpression(nextNode)) { + } else if (nextNode != null && elseBranch == null + && isReturnBlock(thenBranch) && isReturnExpression(nextNode)) { Node thenExpr = null; // if(x)return; return 1 -> return x?void 0:1 if (isReturnExpressBlock(thenBranch)) { @@ -450,17 +451,15 @@ private Node tryMinimizeNot(Node n) { * necessary. */ private Node tryMinimizeExprResult(Node n) { - MinimizedCondition minCond = - MinimizedCondition.fromConditionNode(n.getFirstChild()); - MinimizedCondition.MeasuredNode mNode = + Node originalExpr = n.getFirstChild(); + MinimizedCondition minCond = MinimizedCondition.fromConditionNode(originalExpr); + MeasuredNode mNode = minCond.getMinimized(MinimizationStyle.ALLOW_LEADING_NOT); - Node placeholder = minCond.getPlaceholder(); - if (mNode.getNode().isNot()) { + if (mNode.isNot()) { // Remove the leading NOT in the EXPR_RESULT. - n.replaceChild(placeholder, mNode.getNode().removeFirstChild()); - reportCodeChange(); + replaceNode(originalExpr, mNode.withoutNot()); } else { - replaceNode(placeholder, mNode); + replaceNode(originalExpr, mNode); } return n; } @@ -472,20 +471,19 @@ private Node tryMinimizeExprResult(Node n) { * necessary. */ private Node tryMinimizeHook(Node n) { - MinimizedCondition minCond = - MinimizedCondition.fromConditionNode(n.getFirstChild()); - MinimizedCondition.MeasuredNode mNode = + Node originalCond = n.getFirstChild(); + MinimizedCondition minCond = MinimizedCondition.fromConditionNode(originalCond); + MeasuredNode mNode = minCond.getMinimized(MinimizationStyle.ALLOW_LEADING_NOT); - Node placeholder = minCond.getPlaceholder(); - if (mNode.getNode().isNot()) { + if (mNode.isNot()) { // Swap the HOOK Node thenBranch = n.getSecondChild(); - n.replaceChild(placeholder, mNode.getNode().removeFirstChild()); + replaceNode(originalCond, mNode.withoutNot()); n.removeChild(thenBranch); n.addChildToBack(thenBranch); reportCodeChange(); } else { - replaceNode(placeholder, mNode); + replaceNode(originalCond, mNode); } return n; } @@ -512,21 +510,15 @@ private Node tryMinimizeIf(Node n) { Node thenBranch = originalCond.getNext(); Node elseBranch = thenBranch.getNext(); - MinimizedCondition minCond = - MinimizedCondition.fromConditionNode(originalCond); - originalCond = null; // originalCond was mutated and should not be used. - - Node placeholder = minCond.getPlaceholder(); + MinimizedCondition minCond = MinimizedCondition.fromConditionNode(originalCond); - MinimizedCondition.MeasuredNode unnegatedCond; - MinimizedCondition.MeasuredNode shortCond; // Compute two minimized representations. The first representation counts // a leading NOT node, and the second ignores a leading NOT node. // If we can fold the if statement into a HOOK or boolean operation, // then the NOT node does not matter, and we prefer the second condition. // If we cannot fold the if statement, then we prefer the first condition. - unnegatedCond = minCond.getMinimized(MinimizationStyle.PREFER_UNNEGATED); - shortCond = minCond.getMinimized(MinimizationStyle.ALLOW_LEADING_NOT); + MeasuredNode unnegatedCond = minCond.getMinimized(MinimizationStyle.PREFER_UNNEGATED); + MeasuredNode shortCond = minCond.getMinimized(MinimizationStyle.ALLOW_LEADING_NOT); if (elseBranch == null) { if (isFoldableExpressBlock(thenBranch)) { @@ -535,14 +527,15 @@ private Node tryMinimizeIf(Node n) { // Keep opportunities for CollapseProperties such as // a.longIdentifier || a.longIdentifier = ... -> var a = ...; // until CollapseProperties has been run. - replaceNode(placeholder, unnegatedCond); + replaceNode(originalCond, unnegatedCond); return n; } - if (shortCond.getNode().isNot()) { + if (shortCond.isNot()) { // if(!x)bar(); -> x||bar(); + Node replacementCond = replaceNode(originalCond, shortCond.withoutNot()).detach(); Node or = IR.or( - shortCond.getNode().removeFirstChild(), + replacementCond, expr.removeFirstChild()).srcref(n); Node newExpr = NodeUtil.newExpr(or); parent.replaceChild(n, newExpr); @@ -554,19 +547,18 @@ private Node tryMinimizeIf(Node n) { // Preconditions.checkState(shortCond.isEquivalentTo(unnegatedCond)); // if(x)foo(); -> x&&foo(); - if (isLowerPrecedence(shortCond.getNode(), AND_PRECEDENCE) && - isLowerPrecedence(expr.getFirstChild(), - AND_PRECEDENCE)) { + if (shortCond.isLowerPrecedenceThan(AND_PRECEDENCE) + && isLowerPrecedence(expr.getFirstChild(), AND_PRECEDENCE)) { // One additional set of parentheses is worth the change even if // there is no immediate code size win. However, two extra pair of // {}, we would have to think twice. (unless we know for sure the // we can further optimize its parent. - replaceNode(placeholder, shortCond); + replaceNode(originalCond, shortCond); return n; } - n.removeChild(placeholder); - Node and = IR.and(shortCond.getNode(), expr.removeFirstChild()).srcref(n); + Node replacementCond = replaceNode(originalCond, shortCond).detach(); + Node and = IR.and(replacementCond, expr.removeFirstChild()).srcref(n); Node newExpr = NodeUtil.newExpr(and); parent.replaceChild(n, newExpr); reportCodeChange(); @@ -575,8 +567,7 @@ private Node tryMinimizeIf(Node n) { } else { // Try to combine two IF-ELSE - if (NodeUtil.isStatementBlock(thenBranch) && - thenBranch.hasOneChild()) { + if (NodeUtil.isStatementBlock(thenBranch) && thenBranch.hasOneChild()) { Node innerIf = thenBranch.getFirstChild(); if (innerIf.isIf()) { @@ -584,15 +575,16 @@ private Node tryMinimizeIf(Node n) { Node innerThenBranch = innerCond.getNext(); Node innerElseBranch = innerThenBranch.getNext(); - if (innerElseBranch == null && - !(isLowerPrecedence(unnegatedCond.getNode(), AND_PRECEDENCE) && - isLowerPrecedence(innerCond, AND_PRECEDENCE))) { + if (innerElseBranch == null + && !(unnegatedCond.isLowerPrecedenceThan(AND_PRECEDENCE) + && isLowerPrecedence(innerCond, AND_PRECEDENCE))) { + Node replacementCond = replaceNode(originalCond, unnegatedCond).detach(); n.detachChildren(); n.addChildToBack( IR.and( - unnegatedCond.getNode(), + replacementCond, innerCond.detach()) - .srcref(placeholder)); + .srcref(originalCond)); n.addChildToBack(innerThenBranch.detach()); reportCodeChange(); // Not worth trying to fold the current IF-ELSE into && because @@ -602,7 +594,7 @@ private Node tryMinimizeIf(Node n) { } } } - replaceNode(placeholder, unnegatedCond); + replaceNode(originalCond, unnegatedCond); return n; } @@ -613,8 +605,8 @@ private Node tryMinimizeIf(Node n) { // if(!x)foo();else bar(); -> if(x)bar();else foo(); // An additional set of curly braces isn't worth it. - if (shortCond.getNode().isNot() && !consumesDanglingElse(elseBranch)) { - n.replaceChild(placeholder, shortCond.getNode().removeFirstChild()); + if (shortCond.isNot() && !consumesDanglingElse(elseBranch)) { + replaceNode(originalCond, shortCond.withoutNot()); n.removeChild(thenBranch); n.addChildToBack(thenBranch); reportCodeChange(); @@ -625,7 +617,8 @@ private Node tryMinimizeIf(Node n) { if (isReturnExpressBlock(thenBranch) && isReturnExpressBlock(elseBranch)) { Node thenExpr = getBlockReturnExpression(thenBranch); Node elseExpr = getBlockReturnExpression(elseBranch); - n.removeChild(placeholder); + + Node replacementCond = replaceNode(originalCond, shortCond).detach(); thenExpr.detach(); elseExpr.detach(); @@ -633,7 +626,7 @@ private Node tryMinimizeIf(Node n) { // can be converted to "return undefined;" or some variant, but // that does not help code size. Node returnNode = IR.returnNode( - IR.hook(shortCond.getNode(), thenExpr, elseExpr) + IR.hook(replacementCond, thenExpr, elseExpr) .srcref(n)); parent.replaceChild(n, returnNode); reportCodeChange(); @@ -650,22 +643,22 @@ private Node tryMinimizeIf(Node n) { // if(x)a=1;else a=2; -> a=x?1:2; if (NodeUtil.isAssignmentOp(thenOp)) { Node lhs = thenOp.getFirstChild(); - if (areNodesEqualForInlining(lhs, elseOp.getFirstChild()) && + if (areNodesEqualForInlining(lhs, elseOp.getFirstChild()) // if LHS has side effects, don't proceed [since the optimization // evaluates LHS before cond] // NOTE - there are some circumstances where we can // proceed even if there are side effects... - !mayEffectMutableState(lhs) && - (!mayHaveSideEffects(unnegatedCond.getNode()) || - (thenOp.isAssign() && thenOp.getFirstChild().isName()))) { + && !mayEffectMutableState(lhs) + && (!mayHaveSideEffects(originalCond) + || (thenOp.isAssign() && thenOp.getFirstChild().isName()))) { - n.removeChild(placeholder); + Node replacementCond = replaceNode(originalCond, shortCond).detach(); Node assignName = thenOp.removeFirstChild(); Node thenExpr = thenOp.removeFirstChild(); Node elseExpr = elseOp.getLastChild(); elseOp.removeChild(elseExpr); - Node hookNode = IR.hook(shortCond.getNode(), thenExpr, elseExpr) + Node hookNode = IR.hook(replacementCond, thenExpr, elseExpr) .srcref(n); Node assign = new Node(thenOp.getToken(), assignName, hookNode).srcref(thenOp); Node expr = NodeUtil.newExpr(assign); @@ -677,11 +670,11 @@ private Node tryMinimizeIf(Node n) { } } // if(x)foo();else bar(); -> x?foo():bar() - n.removeChild(placeholder); + Node replacementCond = replaceNode(originalCond, shortCond).detach(); thenOp.detach(); elseOp.detach(); Node expr = IR.exprResult( - IR.hook(shortCond.getNode(), thenOp, elseOp).srcref(n)); + IR.hook(replacementCond, thenOp, elseOp).srcref(n)); parent.replaceChild(n, expr); reportCodeChange(); return expr; @@ -691,8 +684,8 @@ private Node tryMinimizeIf(Node n) { boolean elseBranchIsVar = isVarBlock(elseBranch); // if(x)var y=1;else y=2 -> var y=x?1:2 - if (thenBranchIsVar && elseBranchIsExpressionBlock && - getBlockExpression(elseBranch).getFirstChild().isAssign()) { + if (thenBranchIsVar && elseBranchIsExpressionBlock + && getBlockExpression(elseBranch).getFirstChild().isAssign()) { Node var = getBlockVar(thenBranch); Node elseAssign = getBlockExpression(elseBranch).getFirstChild(); @@ -706,9 +699,8 @@ private Node tryMinimizeIf(Node n) { Preconditions.checkState(name1.hasOneChild()); Node thenExpr = name1.removeFirstChild(); Node elseExpr = elseAssign.getLastChild().detach(); - placeholder.detach(); - Node hookNode = IR.hook(shortCond.getNode(), thenExpr, elseExpr) - .srcref(n); + Node replacementCond = replaceNode(originalCond, shortCond).detach(); + Node hookNode = IR.hook(replacementCond, thenExpr, elseExpr).srcref(n); var.detach(); name1.addChildToBack(hookNode); parent.replaceChild(n, var); @@ -717,8 +709,8 @@ private Node tryMinimizeIf(Node n) { } // if(x)y=1;else var y=2 -> var y=x?1:2 - } else if (elseBranchIsVar && thenBranchIsExpressionBlock && - getBlockExpression(thenBranch).getFirstChild().isAssign()) { + } else if (elseBranchIsVar && thenBranchIsExpressionBlock + && getBlockExpression(thenBranch).getFirstChild().isAssign()) { Node var = getBlockVar(elseBranch); Node thenAssign = getBlockExpression(thenBranch).getFirstChild(); @@ -732,9 +724,8 @@ private Node tryMinimizeIf(Node n) { Node thenExpr = thenAssign.getLastChild().detach(); Preconditions.checkState(name2.hasOneChild()); Node elseExpr = name2.removeFirstChild(); - placeholder.detach(); - Node hookNode = IR.hook(shortCond.getNode(), thenExpr, elseExpr) - .srcref(n); + Node replacementCond = replaceNode(originalCond, shortCond).detach(); + Node hookNode = IR.hook(replacementCond, thenExpr, elseExpr).srcref(n); var.detach(); name2.addChildToBack(hookNode); parent.replaceChild(n, var); @@ -744,7 +735,7 @@ private Node tryMinimizeIf(Node n) { } } - replaceNode(placeholder, unnegatedCond); + replaceNode(originalCond, unnegatedCond); return n; } @@ -822,8 +813,8 @@ private static boolean isFoldableExpressBlock(Node n) { // param, or this doesn't happen. if (calledFn.isGetElem()) { return false; - } else if (calledFn.isGetProp() && - calledFn.getLastChild().getString().startsWith("on")) { + } else if (calledFn.isGetProp() + && calledFn.getLastChild().getString().startsWith("on")) { return false; } } @@ -957,7 +948,7 @@ private static boolean consumesDanglingElse(Node n) { /** * Whether the node type has lower precedence than "precedence" */ - static boolean isLowerPrecedence(Node n, final int precedence) { + static boolean isLowerPrecedence(Node n, int precedence) { return NodeUtil.precedence(n.getToken()) < precedence; } @@ -969,8 +960,8 @@ private static boolean isPropertyAssignmentInExpression(Node n) { new Predicate() { @Override public boolean apply(Node input) { - return (input.isGetProp() && - input.getParent().isAssign()); + return (input.isGetProp() + && input.getParent().isAssign()); } }; @@ -990,7 +981,7 @@ private Node tryMinimizeCondition(Node n) { n = performConditionSubstitutions(n); MinimizedCondition minCond = MinimizedCondition.fromConditionNode(n); return replaceNode( - minCond.getPlaceholder(), + n, minCond.getMinimized(MinimizationStyle.PREFER_UNNEGATED)); } @@ -1050,7 +1041,7 @@ private Node tryReplaceComparisonWithCoercion(Node n, boolean booleanResult) { if (n.getToken() == Token.EQ || n.getToken() == Token.SHEQ) { replacement = IR.not(objExpression); } else { - replacement = booleanResult ? IR.not(IR.not(objExpression)) : objExpression; + replacement = booleanResult ? IR.not(IR.not(objExpression)).srcrefTree(n) : objExpression; } n.replaceWith(replacement); reportCodeChange(); @@ -1146,13 +1137,13 @@ private static boolean isNumberType(Node n) { && type.isNumberValueType(); } - private Node replaceNode(Node lhs, MinimizedCondition.MeasuredNode rhs) { - Node parent = lhs.getParent(); - parent.replaceChild(lhs, rhs.getNode()); - if (rhs.isChanged()) { + private Node replaceNode(Node original, MeasuredNode measuredNodeReplacement) { + if (measuredNodeReplacement.willChange(original)) { + Node replacement = measuredNodeReplacement.applyTo(original); reportCodeChange(); + return replacement; } - return rhs.getNode(); + return original; } /** diff --git a/test/com/google/javascript/jscomp/MinimizedConditionTest.java b/test/com/google/javascript/jscomp/MinimizedConditionTest.java index e772f8c84fc..6d30a60b5ee 100644 --- a/test/com/google/javascript/jscomp/MinimizedConditionTest.java +++ b/test/com/google/javascript/jscomp/MinimizedConditionTest.java @@ -17,14 +17,13 @@ package com.google.javascript.jscomp; import com.google.common.base.Joiner; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.javascript.jscomp.MinimizedCondition.MinimizationStyle; import com.google.javascript.rhino.Node; - -import junit.framework.TestCase; - import java.util.ArrayList; import java.util.List; +import junit.framework.TestCase; /** * Tests for {@link MinimizedCondition} in isolation. @@ -50,17 +49,23 @@ private static Node parseExpr(String code) { return exprResult.getFirstChild(); } + private static Node cloneAttachedTree(Node n) { + Preconditions.checkState(n.getParent().getFirstChild() == n); + return n.getParent().cloneTree().getFirstChild(); + } + private static void minCond(String input, String positive, String negative) { Node inputNode = parseExpr(input); - MinimizedCondition result = MinimizedCondition.fromConditionNode(inputNode); + MinimizedCondition result1 = MinimizedCondition.fromConditionNode(cloneAttachedTree(inputNode)); + MinimizedCondition result2 = MinimizedCondition.fromConditionNode(cloneAttachedTree(inputNode)); Node positiveNode = parseExpr(positive); Node negativeNode = parseExpr(negative); // With counting the leading NOT node: Node positiveResult = - result.getMinimized(MinimizationStyle.PREFER_UNNEGATED).getNode(); + result1.getMinimized(MinimizationStyle.PREFER_UNNEGATED).buildReplacement(); // Without counting the leading NOT node: Node negativeResult = - result.getMinimized(MinimizationStyle.ALLOW_LEADING_NOT).getNode(); + result2.getMinimized(MinimizationStyle.ALLOW_LEADING_NOT).buildReplacement(); if (!positiveResult.isEquivalentTo(positiveNode)) { fail("Not equal:" + "\nExpected: " + positive + diff --git a/test/com/google/javascript/jscomp/PeepholeMinimizeConditionsTest.java b/test/com/google/javascript/jscomp/PeepholeMinimizeConditionsTest.java index 268af380c21..8d110d620f0 100644 --- a/test/com/google/javascript/jscomp/PeepholeMinimizeConditionsTest.java +++ b/test/com/google/javascript/jscomp/PeepholeMinimizeConditionsTest.java @@ -314,6 +314,16 @@ public void testMinimizeDemorgan1() { fold("if(!a&&!b)foo()", "(a||b)||foo()"); } + public void testMinimizeDemorgan2() { + // Make sure trees with cloned functions are marked as changed + fold("(!(a&&!((function(){})())))||foo()", "!a||(function(){})()||foo()"); + } + + public void testMinimizeDemorgan2b() { + // Make sure unchanged trees with functions are not marked as changed + foldSame("!a||(function(){})()||foo()"); + } + public void testMinimizeDemorgan3() { fold("if((!a||!b)&&(c||d)) foo()", "(a&&b||!c&&!d)||foo()"); } @@ -327,9 +337,12 @@ public void testMinimizeDemorgan11() { "(!x || y!==2 && f() || y!==3 && h()) || foo()"); } - public void testMinimizeDemorgan20() { + public void testMinimizeDemorgan20a() { fold("if (0===c && (2===a || 1===a)) f(); else g()", "if (0!==c || 2!==a && 1!==a) g(); else f()"); + } + + public void testMinimizeDemorgan20b() { fold("if (0!==c || 2!==a && 1!==a) g(); else f()", "(0!==c || 2!==a && 1!==a) ? g() : f()"); } @@ -759,9 +772,13 @@ public void testCoercionSubstitution_disabled() { testSame("var x = 1; var y = x != 0;"); } - public void testCoercionSubstitution_booleanResult() { + public void testCoercionSubstitution_booleanResult0() { this.mode = TypeInferenceMode.BOTH; test("var x = {}; var y = x != null;", "var x = {}; var y = !!x;"); + } + + public void testCoercionSubstitution_booleanResult1() { + this.mode = TypeInferenceMode.BOTH; test("var x = {}; var y = x == null;", "var x = {}; var y = !x;"); testSame("var x = {}; var y = x !== null;"); testSame("var x = undefined; var y = x !== null;");