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()) { On 2011/05/31 19:05:24, zundel wrote:
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.
Done. 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())? I'd rather not including anything in this change that I can avoid, the TODO was already there, I'm just making the method accessible. 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) { On 2011/05/31 19:05:24, zundel wrote:
again, a separate method to run 'replaces' might make this simpler.
Done. 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"; On 2011/05/31 19:05:24, zundel wrote:
How about "Cannot resolve unexpected node type"
Done. 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"); On 2011/05/31 19:05:24, zundel wrote:
shouldn't "com.google.gwt.core.client.JavaScriptObject" be a constant
somewhere?
It appears at least twice in this patch.
Done. 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); Yes, but it was only a problem with GwtAstBuilder. Basically, this code assumes that newArray.initializers is mutable, which is not a good assumption anymore. So I'm doing the "right thing" and just replacing the expression entirely. 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"); Nope. :) This wants to go away very quickly, it's only here while both implementations exist, for testing, it should never be documented. 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( I suppose it's six of one, half a dozen of the other. I guess I tend to think of "built-ins" as the code gen types in com.google.gwt.lang. I loosely use the term 'magic' for things that the compiler has to have truly specialized support for. 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>(); On 2011/05/31 19:05:24, zundel wrote:
document 'instantiated'. Too similar to 'seen' for casual reading.
Done. 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>(); On 2011/05/31 19:05:24, zundel wrote:
document 'seen'. Too similar to 'instantiated' for casual reading.
Done. 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(); Just to cache it for quick lookup. I keep getClassFileMapBySource() in a local because addRootTypes() is only called once, so no need to cache an instance field. But this is called often. 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); The incoming root types are by source name, whereas searchForType is by binary name. Updated a couple of variable names to make this more clear. 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()) { It depends on my runAsync change... so you have to sync to a point where runAsync hasn't been rolled back, or else patch the most current version of that change in first. Sorry! 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. On 2011/05/31 19:05:24, zundel wrote:
I assume to save stack - any other reason?
That's it, will document. 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); flowInto() does that internally; the queue is an implementation detail that reduces stack use. http://gwt-code-reviews.appspot.com/1451804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
