Skip to content

Commit 46d729c

Browse files
zbynekzbynek
committed
Applied review suggestions
1 parent 0cfd19b commit 46d729c

3 files changed

Lines changed: 64 additions & 43 deletions

File tree

‎dev/core/src/com/google/gwt/dev/js/JsStaticEval.java‎

Lines changed: 37 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -144,9 +144,12 @@ public boolean visit(JsFunction x, JsContext ctx) {
144144
}
145145
}
146146

147+
/**
148+
* Describes how is the result of evaluation used.
149+
*/
147150
private enum EvalMode {
148-
BOOL,
149-
VOID
151+
BOOL, // result will be coerced to a boolean
152+
VOID // result will be discarded
150153
}
151154

152155
/**
@@ -158,7 +161,11 @@ private enum EvalMode {
158161
*/
159162
private class StaticEvalVisitor extends JsModVisitor {
160163

161-
private Map<JsExpression, EvalMode> evalContext = new HashMap<>();
164+
/**
165+
* Stores how are expression evaluations used.
166+
* Missing entry = no coercion, difference between null and false matters.
167+
*/
168+
private final Map<JsExpression, EvalMode> evalContext = new HashMap<>();
162169

163170
/**
164171
* This is used by {@link #additionCoercesToString}.
@@ -175,9 +182,9 @@ public boolean visit(JsBinaryOperation x, JsContext ctx) {
175182
&& (x.getOperator() == JsBinaryOperator.AND || x.getOperator() == JsBinaryOperator.OR)) {
176183
evalContext.put(x.getArg1(), EvalMode.BOOL);
177184
evalContext.put(x.getArg2(), evalContext.get(x));
178-
}
179-
if (x.getOperator() == JsBinaryOperator.COMMA) {
185+
} else if (x.getOperator() == JsBinaryOperator.COMMA) {
180186
evalContext.put(x.getArg1(), EvalMode.VOID);
187+
evalContext.put(x.getArg2(), evalContext.get(x));
181188
}
182189
return true;
183190
}
@@ -194,15 +201,10 @@ public void endVisit(JsBinaryOperation x, JsContext ctx) {
194201
result = arg1;
195202
} else if (op == JsBinaryOperator.AND) {
196203
result = shortCircuitAnd(x, evalContext);
197-
evalContext.remove(arg1);
198-
evalContext.remove(arg2);
199204
} else if (op == JsBinaryOperator.OR) {
200205
result = shortCircuitOr(x, evalContext);
201-
evalContext.remove(arg1);
202-
evalContext.remove(arg2);
203206
} else if (op == JsBinaryOperator.COMMA) {
204207
result = trySimplifyComma(x);
205-
evalContext.remove(x.getArg1());
206208
} else if (op == JsBinaryOperator.EQ || op == JsBinaryOperator.REF_EQ) {
207209
result = simplifyEqAndRefEq(x);
208210
} else if (op == JsBinaryOperator.NEQ || op == JsBinaryOperator.REF_NEQ) {
@@ -224,7 +226,8 @@ public void endVisit(JsBinaryOperation x, JsContext ctx) {
224226
break;
225227
}
226228
}
227-
229+
evalContext.remove(arg1);
230+
evalContext.remove(arg2);
228231
result = maybeReorderOperations(result);
229232

230233
if (result != x) {
@@ -290,7 +293,13 @@ public void endVisit(JsConditional x, JsContext ctx) {
290293
JsExpression condExpr = x.getTestExpression();
291294
JsExpression thenExpr = x.getThenExpression();
292295
JsExpression elseExpr = x.getElseExpression();
293-
if (condExpr instanceof CanBooleanEval) {
296+
if (condExpr instanceof JsBinaryOperation
297+
&& ((JsBinaryOperation) condExpr).getOperator() == JsBinaryOperator.COMMA) {
298+
JsBinaryOperation condition = (JsBinaryOperation) condExpr;
299+
JsExpression newConditional = accept(new JsConditional(x.getSourceInfo(),
300+
condition.getArg2(), thenExpr, elseExpr));
301+
ctx.replaceMe(withSideEffect(newConditional, condition.getArg1(), x.getSourceInfo()));
302+
} else if (condExpr instanceof CanBooleanEval) {
294303
CanBooleanEval condEval = (CanBooleanEval) condExpr;
295304
if (condEval.isBooleanTrue()) {
296305
JsBinaryOperation binOp = new JsBinaryOperation(x.getSourceInfo(),
@@ -652,8 +661,8 @@ protected static JsExpression shortCircuitAnd(JsBinaryOperation expr,
652661
JsExpression arg2 = expr.getArg2();
653662
if (arg1 instanceof CanBooleanEval) {
654663
CanBooleanEval eval1 = (CanBooleanEval) arg1;
655-
if (eval1.isBooleanTrue() && !arg1.hasSideEffects()) {
656-
return arg2;
664+
if (eval1.isBooleanTrue()) {
665+
return withSideEffect(arg2, arg1, expr.getSourceInfo());
657666
} else if (eval1.isBooleanFalse()) {
658667
return arg1;
659668
}
@@ -662,13 +671,19 @@ protected static JsExpression shortCircuitAnd(JsBinaryOperation expr,
662671
CanBooleanEval eval2 = (CanBooleanEval) arg2;
663672
if (eval2.isBooleanTrue() && !arg2.hasSideEffects()) {
664673
return arg1;
665-
} else if (eval2.isBooleanFalse() && !arg1.hasSideEffects()) {
666-
return arg2;
674+
} else if (eval2.isBooleanFalse()) {
675+
return withSideEffect(arg2, arg1, expr.getSourceInfo());
667676
}
668677
}
669678
return expr;
670679
}
671680

681+
private static JsExpression withSideEffect(JsExpression main, JsExpression sideEffect,
682+
SourceInfo sourceInfo) {
683+
return !sideEffect.hasSideEffects() ? main
684+
: new JsBinaryOperation(sourceInfo, JsBinaryOperator.COMMA, sideEffect, main);
685+
}
686+
672687
/**
673688
* Simplify short circuit OR expressions.
674689
*
@@ -688,18 +703,18 @@ protected static JsExpression shortCircuitOr(JsBinaryOperation expr,
688703
JsExpression arg2 = expr.getArg2();
689704
if (arg1 instanceof CanBooleanEval) {
690705
CanBooleanEval eval1 = (CanBooleanEval) arg1;
691-
if (eval1.isBooleanTrue()) {
706+
if (eval1.isBooleanFalse()) {
707+
return withSideEffect(arg2, arg1, expr.getSourceInfo());
708+
} else if (eval1.isBooleanTrue()) {
692709
return arg1;
693-
} else if (eval1.isBooleanFalse() && !arg1.hasSideEffects()) {
694-
return arg2;
695710
}
696711
}
697712
if (arg2 instanceof CanBooleanEval && evalContext.containsKey(arg1)) {
698713
CanBooleanEval eval2 = (CanBooleanEval) arg2;
699-
if (eval2.isBooleanTrue() && !arg1.hasSideEffects()) {
700-
return arg2;
701-
} else if (eval2.isBooleanFalse() && !arg2.hasSideEffects()) {
714+
if (eval2.isBooleanFalse() && !arg2.hasSideEffects()) {
702715
return arg1;
716+
} else if (eval2.isBooleanTrue()) {
717+
return withSideEffect(arg2, arg1, expr.getSourceInfo());
703718
}
704719
}
705720
return expr;

‎dev/core/src/com/google/gwt/dev/js/ast/JsBinaryOperation.java‎

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
/**
1919
* Represents a JavaScript binary operation.
2020
*/
21-
public final class JsBinaryOperation extends JsExpression implements CanBooleanEval {
21+
public final class JsBinaryOperation extends JsExpression {
2222

2323
private JsExpression arg1;
2424

@@ -68,26 +68,6 @@ public boolean isDefinitelyNull() {
6868
return false;
6969
}
7070

71-
@Override
72-
public boolean isBooleanFalse() {
73-
// assume false && x was already simplified, only check x && false
74-
if (getOperator() == JsBinaryOperator.AND && getArg2() instanceof CanBooleanEval) {
75-
CanBooleanEval eval = (CanBooleanEval) getArg2();
76-
return eval.isBooleanFalse();
77-
}
78-
return false;
79-
}
80-
81-
@Override
82-
public boolean isBooleanTrue() {
83-
// assume true || x was already simplified, only check for x || true
84-
if (getOperator() == JsBinaryOperator.OR && getArg2() instanceof CanBooleanEval) {
85-
CanBooleanEval eval = (CanBooleanEval) getArg2();
86-
return eval.isBooleanTrue();
87-
}
88-
return false;
89-
}
90-
9171
public void setArg1(JsExpression arg1) {
9272
this.arg1 = arg1;
9373
}

‎dev/core/test/com/google/gwt/dev/js/JsStaticEvalTest.java‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,32 @@ public void testShortCircuitAndWithSideEffects() throws Exception {
177177
assertEquals("a&&b();", optimize("!!a && !!b()"));
178178
assertEquals("a();", optimize("a() && false && c();"));
179179
assertEquals("a(),e();", optimize("a() && false && c() ? d() : e();"));
180+
assertEquals("a()&&c()?d():e();", optimize("a() && true && c() ? d() : e();"));
181+
}
182+
183+
public void testShortCircuitOr() throws Exception {
184+
assertEquals("alert(true);", optimize("alert(true || a)"));
185+
assertEquals("alert(a);", optimize("alert(false || a)"));
186+
187+
// these can't be simplified to maintain type
188+
assertEquals("alert(a||true);", optimize("alert(a || true)"));
189+
assertEquals("alert(a||false);", optimize("alert(a || false)"));
190+
assertEquals("alert(!!a||!!b);", optimize("alert(!!a || !!b)"));
191+
192+
// in boolean context we can simplify more
193+
assertEquals("alert(a||b?c:d);", optimize("alert(!!a || !!b ? c :d)"));
194+
assertEquals("alert(c);", optimize("alert(true || !!b ? c :d)"));
195+
assertEquals("alert(b?c:d);", optimize("alert(false || !!b ? c :d)"));
196+
assertEquals("alert(b?c:d1);", optimize("alert(b || false ? c :d1)"));
197+
assertEquals("alert(c1);", optimize("alert(b || true ? c1 :d)"));
198+
assertEquals("alert(c2);", optimize("alert(a || true || b ? c2 :d)"));
199+
}
200+
201+
public void testShortCircuitOrWithSideEffects() throws Exception {
202+
assertEquals("a||b();", optimize("!!a || !!b()"));
203+
assertEquals("a();", optimize("a() || true || c();"));
204+
assertEquals("a(),d();", optimize("a() || true || c() ? d() : e();"));
205+
assertEquals("a()||c()?d():e();", optimize("a() || false || c() ? d() : e();"));
180206
}
181207

182208
public void testLiteralEqNull() throws Exception {

0 commit comments

Comments
 (0)