http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/ast/JDeclaredType.java File dev/core/src/com/google/gwt/dev/jjs/ast/JDeclaredType.java (right):
http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/ast/JDeclaredType.java#newcode248 dev/core/src/com/google/gwt/dev/jjs/ast/JDeclaredType.java:248: if (getClass().desiredAssertionStatus()) { Seems like you could have implemented your assertion in a separate method (like you did for the static versionof JType.replaces()) and then the resolve() method just turns into a single assertion and copying two lists. http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java File dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java (right): http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java#newcode58 dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java:58: // TODO: replace with BinaryName.toSourceName(type.getName())? Not that it is functionally any different, but yes I think we should go ahead and do it. If we ever decide to fix the 'allow $ in java name' consolidating references to a single method will help. http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/ast/JNameOf.java File dev/core/src/com/google/gwt/dev/jjs/ast/JNameOf.java (right): http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/ast/JNameOf.java#newcode52 dev/core/src/com/google/gwt/dev/jjs/ast/JNameOf.java:52: if (node instanceof JType) { again, a separate method to run 'replaces' might make this simpler. http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/ast/JNameOf.java#newcode59 dev/core/src/com/google/gwt/dev/jjs/ast/JNameOf.java:59: assert false : "Should not get here"; How about "Cannot resolve unexpected node type" http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/ast/js/JsniMethodRef.java File dev/core/src/com/google/gwt/dev/jjs/ast/js/JsniMethodRef.java (right): http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/ast/js/JsniMethodRef.java#newcode62 dev/core/src/com/google/gwt/dev/jjs/ast/js/JsniMethodRef.java:62: assert jsoType.getName().equals("com.google.gwt.core.client.JavaScriptObject"); shouldn't "com.google.gwt.core.client.JavaScriptObject" be a constant somewhere? It appears at least twice in this patch. http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java File dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java (right): http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java#newcode505 dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java:505: JArrayType arrayType = program.getTypeArray(JPrimitiveType.INT); Is this change related to code stitching? http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java File dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java (right): http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java#newcode2761 dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java:2761: public static boolean ENABLED = System.getProperties().containsKey("x.gwt.astBuilder"); why prefix with 'x'? Isn't an undocumented java system property obscure enough for you? :-) http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java File dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java (right): http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java#newcode465 dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java:465: private static final Set<String> MAGIC_METHOD_CALLS = new LinkedHashSet<String>(Arrays.asList( Is "Magic" really the right terminology? I usually see these kinds of methods called 'built-ins' http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java#newcode477 dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java:477: private final Set<JDeclaredType> instantiated = new IdentityHashSet<JDeclaredType>(); document 'instantiated'. Too similar to 'seen' for casual reading. http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java#newcode485 dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java:485: private final Set<JNode> seen = new IdentityHashSet<JNode>(); document 'seen'. Too similar to 'instantiated' for casual reading. http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java#newcode497 dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java:497: this.classFileMap = rpo.getCompilationState().getClassFileMap(); do you need to keep this as a separate field? (you don't for getClassFileMapBySource() ) http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java#newcode504 dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java:504: JDeclaredType type = program.getFromTypeMap(rootType); Why not use "searchForType()" here? http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java#newcode522 dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java:522: for (JMethod entryMethod : program.getEntryMethods()) { Patched in your change and I can't find this method. http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java#newcode722 dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java:722: // Queue up visit / resolve on the body. I assume to save stack - any other reason? http://gwt-code-reviews.appspot.com/1451804/diff/1/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java#newcode870 dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java:870: flowInto((JMethod) node); why not queue up with todo.add()? http://gwt-code-reviews.appspot.com/1451804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
