Lots of good changes in here we really need to get in! How much does this save on code size?
Mostly LGTM with a few suggestions, nits, and confusions; nothing major. http://gwt-code-reviews.appspot.com/62805/diff/1/21 File dev/core/src/com/google/gwt/dev/jjs/ast/JBinaryOperation.java (right): http://gwt-code-reviews.appspot.com/62805/diff/1/21#newcode23 Line 23: public class JBinaryOperation extends JExpression { I assume you're deleting HasSettableType? Don't see the deletion in this patch. http://gwt-code-reviews.appspot.com/62805/diff/1/9 File dev/core/src/com/google/gwt/dev/jjs/ast/JNewArray.java (right): http://gwt-code-reviews.appspot.com/62805/diff/1/9#newcode102 Line 102: public JType getType() { Should be able to use covariant return type to return a JNonNullType. http://gwt-code-reviews.appspot.com/62805/diff/1/13 File dev/core/src/com/google/gwt/dev/jjs/ast/JNewInstance.java (right): http://gwt-code-reviews.appspot.com/62805/diff/1/13#newcode39 Line 39: public JType getType() { Should be able to use covariant return type to return a JNonNullType. http://gwt-code-reviews.appspot.com/62805/diff/1/18 File dev/core/src/com/google/gwt/dev/jjs/ast/JNonNullType.java (right): http://gwt-code-reviews.appspot.com/62805/diff/1/18#newcode28 Line 28: public JNonNullType(JReferenceType ref) { Should be restricted, only JProgram should create since they're interned. http://gwt-code-reviews.appspot.com/62805/diff/1/11 File dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java (right): http://gwt-code-reviews.appspot.com/62805/diff/1/11#newcode702 Line 702: JReferenceType tGreater = classify1 > classify2 ? type1 : type2; Holy cow... is this just a straight up bug? Should we commit this fix immediately as a separate change? http://gwt-code-reviews.appspot.com/62805/diff/1/11#newcode1205 Line 1205: public JReferenceType strongerType(JReferenceType type1, JReferenceType type2) { BTW: I completely dropped the ball on javadoc here, making it very hard to understand the precise semantics. I assume based on your changes that you do understand, would you mind elucidating? Thanks! http://gwt-code-reviews.appspot.com/62805/diff/1/15 File dev/core/src/com/google/gwt/dev/jjs/ast/JTypeOracle.java (right): http://gwt-code-reviews.appspot.com/62805/diff/1/15#newcode416 Line 416: for (int i = 0; i < program.getDeclaredTypes().size(); ++i) { All 4 of these can just be turned into enhanced for loops while you're in there. http://gwt-code-reviews.appspot.com/62805/diff/1/15#newcode823 Line 823: private boolean isInstantiatedType(JReferenceType type, You know what, this whole instantiated types mess where we pass around parameters that shadow the instance field is completely bogus. If you're going to make this non-static, how about we just fix all the places where we pass an 'instantiatedTypes' param and always use the field. I think it's literally always the same object. http://gwt-code-reviews.appspot.com/62805/diff/1/27 File dev/core/src/com/google/gwt/dev/jjs/impl/EqualityNormalizer.java (right): http://gwt-code-reviews.appspot.com/62805/diff/1/27#newcode90 Line 90: if (lhsStatus != StringStatus.NONNULL This gets to be a little confusing. Maybe we should make USE_TRIPLE_EQUALS equal '2' if either one is a non-null, then a switch statement can differentiate the cases. Because in that case, no replacement even needs to be done, right? http://gwt-code-reviews.appspot.com/62805/diff/1/27#newcode152 Line 152: if (lhs.getType() instanceof JPrimitiveType) { Can either of these two new if tests actually get hit? I'd have thought the surrounding code would make it impossible. Should these be assertions? http://gwt-code-reviews.appspot.com/62805/diff/1/42 File dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java (left): http://gwt-code-reviews.appspot.com/62805/diff/1/42#oldcode553 Line 553: typeList.add(typeNull); If we don't need this code, the comment should also go. http://gwt-code-reviews.appspot.com/62805/diff/1/42 File dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java (right): http://gwt-code-reviews.appspot.com/62805/diff/1/42#newcode129 Line 129: && method != program.getNullMethod()) { LG, but change related? http://gwt-code-reviews.appspot.com/62805/diff/1/42#newcode409 Line 409: } else if (triviallyFalse && toType != program.getTypeNull()) { I was slightly nervous about the changes in JTypeOracle.canTriviallyCast/canTheoreticallyCast with respect to nullity, and I think this is why. I'm sure the code is right, but I'm struggling to reason about it. http://gwt-code-reviews.appspot.com/62805/diff/1/42#newcode418 Line 418: // TODO(nullable): I don't understand this. I still don't understand this. LOL. http://gwt-code-reviews.appspot.com/62805 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
