Responses below. I'll upload a new patch in a moment. I still need to get size updates and to try and recover the reason why TypeTightener has this new code checking for a cast that only checks for nullness. I'm guessing the extra cast caused worse code generation somewhere.
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 { It's deleted; I must have forgotten to include the deletion in the patch. I don't remember the exact reason, but it was probably that there are no users of it. 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() { On 2009/09/11 22:35:44, scottb wrote: > Should be able to use covariant return type to return a JNonNullType. Done. 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() { On 2009/09/11 22:35:44, scottb wrote: > Should be able to use covariant return type to return a JNonNullType. Done. 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) { I agree. It's default access now. 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; Yes, I keep forgetting to send around the one-character patch. Shall I take that as a LGTM and put that one in alone? http://gwt-code-reviews.appspot.com/62805/diff/1/11#newcode1205 Line 1205: public JReferenceType strongerType(JReferenceType type1, JReferenceType type2) { Done. I've been thinking of it as greatest lower bound. Likewise for generalizeType being least upper bound, which I'll document as well. 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) { Hmm, yes. Done. http://gwt-code-reviews.appspot.com/62805/diff/1/15#newcode823 Line 823: private boolean isInstantiatedType(JReferenceType type, I agree that it could be cleaner. It's easy to call the wrong one of these methods. Cleaning it up is not as simple as always using the field, though. ControlFlowAnalyzer builds up its own new instantiatedTypes set as it goes, and while it's working it passes that intermediate set to all the isInstantiated static methods. This is helpful already for the Pruner, but it's absolutely vital for the code splitter, because the code splitter reasons about lots of ControlFlowAnalyzers that don't cover the whole program. One possible angle to clean it up would be if CFA could build a separate JTypeOracle corresponding to the program subset it thinks is live? Then it could be a field again. Instead of CFA having its own instantiableTypes set, it would have its own JTypeOracle. I wonder how that would work out? 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 That works out well. Check out the updated patch with three comparison strategies and a switch statement. I reordered the numbers so that their strategy names sort in numeric order. http://gwt-code-reviews.appspot.com/62805/diff/1/27#newcode152 Line 152: if (lhs.getType() instanceof JPrimitiveType) { Hmm, yes, you're right. I changed it to an assertion. 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); Oops, done. The problem, by the way, is that null doesn't always lose; it, combined with a non-null type, would lead to a nullable result. A "nothing" type would be convenient here, a type not even including null. 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#newcode409 Line 409: } else if (triviallyFalse && toType != program.getTypeNull()) { The issue is that if it's already a cast to null, don't do anything. Previously that didn't come up, because a cast to null could never be trivially false. Now, however, it's possible. For example, a cast of a string literal to the null type will always fail, and the compiler can now predict that it will always fail. Part of the complexity here is that we don't actually implement nullity checks in web mode. The way I've been reasoning about it is to implement the optimizers as if those checks actually occurred, even though in fact the nullness part of casts is dropped later on. Maybe we can come up with a better way of looking at it, but that's the reasoning so far in this patch. http://gwt-code-reviews.appspot.com/62805/diff/1/42#newcode418 Line 418: // TODO(nullable): I don't understand this. Hmm, this effectively drops cast operations. I will try to recover why this was added and update the comment. http://gwt-code-reviews.appspot.com/62805 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
