http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java File dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java (right):
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode246 dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:246: assert jsprogram.getFragmentCount() == 1; extra indentation, here and several places below in this method http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode261 dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:261: stat = result; this assignment is redundant, we have the same assignment below under the "if (keepIt)" block below. http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode345 dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:345: */ It looks like all this does is maybe remove some ctors (and if any ctors remain, set a flag). Would a better name be "maybeRemoveCtorsFromDefineSeedStmt()"? http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode357 dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:357: JConstructor ctor = (JConstructor) maybeCtor; how can ctor be null here? http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode504 dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:504: JsNameRef lhs = (JsNameRef) binExpr.getArg1(); Is there a reason for the change in the way of testing that we are using the "_" tempVar. E.g. why no longer do: if (!lhs.getName().getShortIdent().equals("_")) Is it just for brevity? Or is there a requirement that the underBar be existing at this point (and is this a new requirement?). http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java (left): http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#oldcode1255 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1255: why get rid of this comment? http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java (right): http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode572 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:572: } Makes sense, but take note that this is only currently enabled for PRETTY or DETAILED output modes. http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode610 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:610: white space http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode724 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:724: Are these checks needed here, since the corresponding visit(JClassType...) method returns false if x is the class literal holder, or is an immortal code gen type. http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1438 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1438: if (program.immortalCodeGenTypes.contains(x)) { s/Handle/Handled http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1665 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1665: } Suggest change to return a new empty JsLiteralObject here, if castMap is null (since this is what all callers must do anyway). http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1816 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1816: for (JMethod method : x.getMethods()) { Should this maybe be an assertion, that no immortal types should be allowed to have virtual methods or constructors? Otherwise it might be confusing if they are routinely thrown out? Should there also be a comment, similar to the one in JProgram where immortal gen types are defined, stating that immortal types should not have any non-static methods or fields? http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1821 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1821: if (JProgram.isClinit(method)) { Why do we need to generate an empty clinit? Is it guaranteed to be empty since we don't allow any non-static fields? Add an assert that method is empty and contains no statements? Add a comment? http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1837 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1837: for (JField field : x.getFields()) { Assertion instead? http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1845 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1845: JExpression init = field.getInitializer(); Comment here, explaining that immortal types can only have non literal initializers for jso's that are instances of createArray or createObject, etc. http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1848 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1848: // no literal, but it could be a JavaScriptObject If it's not a JSO, do we allow any initializer method? I believe in the comment in JProgram, it says that the initializer can be primitive, a literal, or one of the 2 JSO.create methods below. Should there be an else check that it's a primitive initializer here? http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1904 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1904: JsExpression castMap = generateCastableTypeMap(x); can we just have generateCastableTypeMap() return a new JsObjectLiteral if it's null, instead of handle here? http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode2014 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:2014: white space http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java File dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java (right): http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java#newcode146 dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java:146: * <pre> s/JSeedIdOff/JSeedIdOf http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java#newcode267 dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java:267: private void execImpl() { first? then what? Consider removing "first" in the comment http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java File dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java (right): http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java#newcode633 dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java:633: /** Update comment, since this is used for both regular codeGenTypes and immortal types. Consider changing the name of this method to "traverseTypes", etc. http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceGetClassOverrides.java File dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceGetClassOverrides.java (right): http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceGetClassOverrides.java#newcode25 dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceGetClassOverrides.java:25: Need trailing '.' Also, add explanation for special handling for JSO's. E.g. "Prune all overrides of Object.getClass(), except for when the enclosing class is JavaScriptObject. (and explain why?). How do instances of String.getClass() get handled? http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceGetClassOverrides.java#newcode34 dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceGetClassOverrides.java:34: The name of this class at first made me think this was going to remove inlines of getClass()....How about "GetClassReplacer" (or some such). http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceGetClassOverrides.java#newcode61 dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceGetClassOverrides.java:61: } s/overriden/overridden http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/js/JsSourceGenerationVisitorWithSizeBreakdown.java File dev/core/src/com/google/gwt/dev/js/JsSourceGenerationVisitorWithSizeBreakdown.java (right): http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/js/JsSourceGenerationVisitorWithSizeBreakdown.java#newcode65 dev/core/src/com/google/gwt/dev/js/JsSourceGenerationVisitorWithSizeBreakdown.java:65: } white space http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/js/JsStackEmulator.java File dev/core/src/com/google/gwt/dev/js/JsStackEmulator.java (right): http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/js/JsStackEmulator.java#newcode584 dev/core/src/com/google/gwt/dev/js/JsStackEmulator.java:584: @Override Shouldn't this be visit(JsFunction,...)? http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/js/JsToStringGenerationVisitor.java File dev/core/src/com/google/gwt/dev/js/JsToStringGenerationVisitor.java (right): http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/js/JsToStringGenerationVisitor.java#newcode883 dev/core/src/com/google/gwt/dev/js/JsToStringGenerationVisitor.java:883: } whitespace http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/js/ast/JsSeedIdOf.java File dev/core/src/com/google/gwt/dev/js/ast/JsSeedIdOf.java (right): http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/js/ast/JsSeedIdOf.java#newcode20 dev/core/src/com/google/gwt/dev/js/ast/JsSeedIdOf.java:20: /** Is this the full description? How is it different from JsNameOf? Just a placeholder for the special seedId? http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/SeedUtil.java File dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/SeedUtil.java (right): http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/SeedUtil.java#newcode29 dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/SeedUtil.java:29: reformat line lengths here! http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/SeedUtil.java#newcode40 dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/SeedUtil.java:40: public static native JavaScriptObject defineSeed(int id, int superSeed, JavaScriptObject castableTypeMap) /*-{ It looks like clazz is unused here http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/SeedUtil.java#newcode41 dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/SeedUtil.java:41: var seed = @com.google.gwt.lang.SeedUtil::seedTable[id], clazz; move this comment inside if clause http://gwt-code-reviews.appspot.com/1447821/diff/3001/user/super/com/google/gwt/emul/java/lang/Class.java File user/super/com/google/gwt/emul/java/lang/Class.java (right): http://gwt-code-reviews.appspot.com/1447821/diff/3001/user/super/com/google/gwt/emul/java/lang/Class.java#newcode45 user/super/com/google/gwt/emul/java/lang/Class.java:45: Class<T> clazz = new Class<T>(); shouldn't this call setName with seedId (and not -1)? http://gwt-code-reviews.appspot.com/1447821/diff/3001/user/super/com/google/gwt/emul/java/lang/Class.java#newcode147 user/super/com/google/gwt/emul/java/lang/Class.java:147: if (seedId > -1) { s/pruned b using/pruned by using? http://gwt-code-reviews.appspot.com/1447821/diff/3001/user/super/com/google/gwt/emul/java/lang/Class.java#newcode211 user/super/com/google/gwt/emul/java/lang/Class.java:211: white space http://gwt-code-reviews.appspot.com/1447821/diff/3001/user/super/com/google/gwt/emul/java/lang/Object.java File user/super/com/google/gwt/emul/java/lang/Object.java (right): http://gwt-code-reviews.appspot.com/1447821/diff/3001/user/super/com/google/gwt/emul/java/lang/Object.java#newcode30 user/super/com/google/gwt/emul/java/lang/Object.java:30: */ Is this not true for other fields in this class? http://gwt-code-reviews.appspot.com/1447821/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
