http://gwt-code-reviews.appspot.com/436801/diff/1/3 File dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumption.java (right):
http://gwt-code-reviews.appspot.com/436801/diff/1/3#newcode157 dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumption.java:157: ConstantsAssumption other = (ConstantsAssumption) obj; On 2010/05/03 15:15:17, Lex wrote:
It won't come up much in practice, but it seems like there should be a
type test
here before doing the cast. If the object is the wrong type, return
false. I'd prefer to have a class cast here. http://gwt-code-reviews.appspot.com/436801/diff/1/3#newcode158 dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumption.java:158: return values.equals(other.values); On 2010/05/03 15:15:17, Lex wrote:
Shouldn't this apply equal(JValueLiteral,JValueLiteral) to each
element?
Otherwise it will return false unnecessarily.
Nice catch. Done. http://gwt-code-reviews.appspot.com/436801/diff/1/3#newcode188 dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumption.java:188: return null; On 2010/05/03 15:15:17, Lex wrote:
Null is the bottom assumption. Shouldn't this case return the
receiver? You are right. Done. Test fixed. http://gwt-code-reviews.appspot.com/436801/diff/1/3#newcode211 dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumption.java:211: if (this == TOP) { On 2010/05/03 15:15:17, Lex wrote:
This should ideally check isEmpty(), too. Or else, make the class
constructor
and the set() method private, so that isEmpty() isn't needed.
Done. http://gwt-code-reviews.appspot.com/436801/diff/1/6 File dev/core/test/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAnalysisTest.java (right): http://gwt-code-reviews.appspot.com/436801/diff/1/6#newcode1 dev/core/test/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAnalysisTest.java:1: package com.google.gwt.dev.jjs.impl.gflow.constants; On 2010/05/03 15:15:17, Lex wrote:
This files is also missing a copyright header.
Done. http://gwt-code-reviews.appspot.com/436801/diff/1/7 File dev/core/test/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumptionTest.java (right): http://gwt-code-reviews.appspot.com/436801/diff/1/7#newcode1 dev/core/test/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAssumptionTest.java:1: package com.google.gwt.dev.jjs.impl.gflow.constants; On 2010/05/03 15:15:17, Lex wrote:
Needs copyright header.
Done. http://gwt-code-reviews.appspot.com/436801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
