All done in: http://gwt-code-reviews.appspot.com/143812/show
http://gwt-code-reviews.appspot.com/130812/diff/1/2 File dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/AssumptionDeducer.java (right): http://gwt-code-reviews.appspot.com/130812/diff/1/2#newcode135 Line 135: // Only last expression can be reverse engineered. On 2010/02/08 16:16:14, Lex wrote:
Either htis comment is innacurate or I don't understand what's going
on. I
believe what's going on is that, from the point of view of a CFG, the
only
operation under consideration is the passing of the result of the last expression to being the result of the multi-expression. All the other evaluation steps are represented as separate CFG nodes, and those
nodes' effects
on the assumptions will already be presented in the assumptions list.
If that's what's going on, then please update the comment.
Comment updated. http://gwt-code-reviews.appspot.com/130812/diff/1/2#newcode154 Line 154: private boolean isDeduciveEqValue(JExpression e) { On 2010/02/08 16:16:14, Lex wrote:
This method seems to mean, "would two expressions equal to e be
substitutable
for each other". If that's what it means, please update the name and
comment.
"Can deduce something" is awfully vague.
Done. http://gwt-code-reviews.appspot.com/130812/diff/1/4 File dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAnalysis.java (right): http://gwt-code-reviews.appspot.com/130812/diff/1/4#newcode30 Line 30: * As of now supports only locals & parameters. On 2010/02/08 16:16:14, Lex wrote:
Document that it also does constant folding.
It actually doesn't. (As of now). http://gwt-code-reviews.appspot.com/130812/diff/1/5 File dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumption.java (right): http://gwt-code-reviews.appspot.com/130812/diff/1/5#newcode35 Line 35: public static class CopyOnWrite { On 2010/02/08 16:16:14, Lex wrote:
It took me a long time to figure out that this class is a builder that
is
initialized with a previous instance of the class.
That's a good pattern, but the name makes it obscure. Please rename
it to
something with something including "builder" or "updater" or the like.
Done. http://gwt-code-reviews.appspot.com/130812/diff/1/5#newcode59 Line 59: public ConstantsAssumption unwrap() { BTW, I'm not fond of "unwrap" name, but using "get" is PITA due to find usages/rename becoming really slow. Any suggestions? http://gwt-code-reviews.appspot.com/130812/diff/1/5#newcode154 Line 154: values.put(variable, literal); On 2010/02/08 16:16:14, Lex wrote:
Given that there is a builder for the class, it would seem better to
make
mutators like this be private. Then, the public API of the class
would be
immutable.
Likewise for the assumption/CopyOnWrite pairs for all the other
optimizers. Done. http://gwt-code-reviews.appspot.com/130812/diff/1/5#newcode185 Line 185: private boolean equal(Object o1, Object o2) { On 2010/02/08 16:16:14, Lex wrote:
This ends up compalir JValueLiteral AST nodes rather than the
represented
literals.
Perhaps change the argument types to JValueLiteral, and compare the
underlying
literals? Be careful with floats; for safety's sake we might start
with only
treating them as equal if their underlying bits are the same.
Done. http://gwt-code-reviews.appspot.com/130812 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
