http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/ast/JClassType.java File dev/core/src/com/google/gwt/dev/jjs/ast/JClassType.java (left):
http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/ast/JClassType.java#oldcode90 dev/core/src/com/google/gwt/dev/jjs/ast/JClassType.java:90: * I assume this is no longer relevant? http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/ast/JField.java File dev/core/src/com/google/gwt/dev/jjs/ast/JField.java (right): http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/ast/JField.java#newcode178 dev/core/src/com/google/gwt/dev/jjs/ast/JField.java:178: boolean replaces(JField originalField) { parens around && clause for better readability/formatting http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/ast/JFieldRef.java File dev/core/src/com/google/gwt/dev/jjs/ast/JFieldRef.java (right): http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/ast/JFieldRef.java#newcode103 dev/core/src/com/google/gwt/dev/jjs/ast/JFieldRef.java:103: public void resolve(JField newField) { A bit more readable to use 'target' rather than get(field) in the assert here. http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java File dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java (right): http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java#newcode316 dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java:316: assert returnType.replaces(this.returnType); overrides is not a param here, is this still relevant? http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java#newcode437 dev/core/src/com/google/gwt/dev/jjs/ast/JMethod.java:437: boolean replaces(JMethod originalMethod) { parens around && clause for readability http://gwt-code-reviews.appspot.com/1451804/diff/2002/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/2002/dev/core/src/com/google/gwt/dev/jjs/ast/JNameOf.java#newcode29 dev/core/src/com/google/gwt/dev/jjs/ast/JNameOf.java:29: if (newElement instanceof JType) { maybe add assertions here for the type of oldElement in each case, e.g.: assert(oldElement instanceof JType) ... assert(oldElement instanceof JField) ... assert(oldElement instanceof JMethod) http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/ast/JNameOf.java#newcode84 dev/core/src/com/google/gwt/dev/jjs/ast/JNameOf.java:84: /** s/references/reference http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/ast/JReferenceType.java File dev/core/src/com/google/gwt/dev/jjs/ast/JReferenceType.java (right): http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/ast/JReferenceType.java#newcode76 dev/core/src/com/google/gwt/dev/jjs/ast/JReferenceType.java:76: public boolean replaces(JType originalType) { assert(originalType instanceof JReferenceType)? http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/ast/JType.java File dev/core/src/com/google/gwt/dev/jjs/ast/JType.java (right): http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/ast/JType.java#newcode87 dev/core/src/com/google/gwt/dev/jjs/ast/JType.java:87: boolean replaces(JType originalType) { might read/format more clearly if you add parens around the && clause http://gwt-code-reviews.appspot.com/1451804/diff/2002/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/2002/dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java#newcode488 dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java:488: /** maybe reference where AsyncFragmentLoader is, e.g. (see {@link com.google.gwt.core.client.impl.AsyncFragmentLoader}). http://gwt-code-reviews.appspot.com/1451804/diff/2002/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/2002/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java#newcode2760 dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java:2760: usually we've been naming our system properties like "gwt.*" rather than "x.gwt.*" (no biggy)....suggestion: "gwt.enableAstBuilder" http://gwt-code-reviews.appspot.com/1451804/diff/2002/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/2002/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java#newcode100 dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java:100: * need TODO for clinit traversal? explain? http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java#newcode104 dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java:104: * s/method/Method http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java#newcode107 dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java:107: * - New operations - type instantiability, constructor, overrides of live is "- GWT.create()" meant to be a separate bullet point? Or is it another in the list, in which case s/-/, http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java#newcode113 dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java:113: * Should this be part of GWT.runAsync() bullet point above? http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java#newcode151 dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java:151: shouldn't this comment be moved down to block dealing with enums? Maybe a separate comment here dealing with JArrayType. What's the meaning of the reference to "ImplementClassLiteralsAsFields" here? Should it be "rescue enumType.values()/valueOf", since we are not talking about the Enum.valueOf() method defined on the super class Enum. http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java#newcode276 dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java:276: public void endVisit(JNewInstance x, Context ctx) { why super here? http://gwt-code-reviews.appspot.com/1451804/diff/2002/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java#newcode369 dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java:369: } this is a repeated check for "(answerType == null)", did you mean something else here? http://gwt-code-reviews.appspot.com/1451804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
