This is a great change! It simplifies the setup code for constant analysis, and it means that the common case of a top assumption doesn't require an explicit entry in the map.
Some small changes are requested in the implementation. http://gwt-code-reviews.appspot.com/436801/diff/1/2 File dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAnalysis.java (right): http://gwt-code-reviews.appspot.com/436801/diff/1/2#newcode49 dev/core/src/com/google/gwt/dev/jjs/impl/gflow/constants/ConstantsAnalysis.java:49: ConstantsAssumption.TOP, assumptionMap); Nice to see this part go back to being so simple! 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; 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. 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); Shouldn't this apply equal(JValueLiteral,JValueLiteral) to each element? Otherwise it will return false unnecessarily. 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; Null is the bottom assumption. Shouldn't this case return the receiver? 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) { This should ideally check isEmpty(), too. Or else, make the class constructor and the set() method private, so that isEmpty() isn't needed. 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; This files is also missing a copyright header. 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; Needs copyright header. http://gwt-code-reviews.appspot.com/436801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
