LGTM (w/minor cleanup + a suggestion)
I like the management of class literal rescues much better now!


http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
(right):

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode93
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:93:
whitespace

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode600
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:600:
instantiatedTypes.add(type);
Suggest combining all this into 1 method call to something like
("maybeRescueClassLiteral(type)"), and then mRCL contains logic to see
if classLiteralsToBeRescuedIfGetClassLive is null or not, to decide
whether to rescue or mark for rescue.

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode763
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:763:
suggest (see above) changing this to maybeRescueClassLiteral(type), and
have it check whether classLiteralsToBeRescuedIfGetClassLive is null or
not.  This way, the logic to decide whether to rescue immediately or
deferred is separated from the logic for whether getClass is live, etc.

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode769
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:769:
if (classLiteralsToBeRescuedIfGetClassIsLive != null) {
is re-entrant the right term (I usually think of it wrt to concurrency)?

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode870
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:870:
whitespace

http://gwt-code-reviews.appspot.com/1447821/diff/15001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode917
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:917:
/**
C

http://gwt-code-reviews.appspot.com/1447821/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

Reply via email to