Mostly LG, some comments. Also mostly agree with Lex's comments, too. I called out a couple in particular that are sort of style questions where I agree.
http://gwt-code-reviews.appspot.com/160801/diff/16001/17002 File dev/core/src/com/google/gwt/core/ext/linker/SoftPermutation.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17002#newcode25 dev/core/src/com/google/gwt/core/ext/linker/SoftPermutation.java:25: public abstract class SoftPermutation extends Artifact<SoftPermutation> { Agreed. http://gwt-code-reviews.appspot.com/160801/diff/16001/17002#newcode34 dev/core/src/com/google/gwt/core/ext/linker/SoftPermutation.java:34: public abstract CompilationResult getCompilationResult(); Agreed. http://gwt-code-reviews.appspot.com/160801/diff/16001/17003 File dev/core/src/com/google/gwt/core/ext/linker/impl/SelectionScriptLinker.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17003#newcode419 dev/core/src/com/google/gwt/core/ext/linker/impl/SelectionScriptLinker.java:419: protected abstract String getModuleSuffix(TreeLogger logger, LinkerContext context) Heh, for me this formats the way it already is. http://gwt-code-reviews.appspot.com/160801/diff/16001/17009 File dev/core/src/com/google/gwt/dev/Permutation.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17009#newcode58 dev/core/src/com/google/gwt/dev/Permutation.java:58: private Object readResolve() throws ObjectStreamException { Unnecessary throws. http://gwt-code-reviews.appspot.com/160801/diff/16001/17009#newcode66 dev/core/src/com/google/gwt/dev/Permutation.java:66: OracleComparator.INSTANCE); I tend to agree. The merges should happen deterministically, right? http://gwt-code-reviews.appspot.com/160801/diff/16001/17010 File dev/core/src/com/google/gwt/dev/Precompile.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17010#newcode600 dev/core/src/com/google/gwt/dev/Precompile.java:600: // Construct a key from the stringified map of live rebind answers. Per my comment on 686, this is basically the wrong key. We need a string that looks like {request1=[result1, result2],...} http://gwt-code-reviews.appspot.com/160801/diff/16001/17010#newcode686 dev/core/src/com/google/gwt/dev/Precompile.java:686: mergeInto.putRebindAnswer("com.google.gwt.lang.CollapsedPropertyHolder", This smells bad to me. I think the problem is that the logic that merges hard perms together needs to be updated. Basically, that logic needs something like CollapsedPermutationKey itself, so that the key generated is not based simply on the "canonical" property oracle, but rather is a function of all the soft rebinds. I agree with Lex that the idea of a canonical property oracle ought to die. http://gwt-code-reviews.appspot.com/160801/diff/16001/17011 File dev/core/src/com/google/gwt/dev/cfg/BindingProperty.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17011#newcode272 dev/core/src/com/google/gwt/dev/cfg/BindingProperty.java:272: Map<String, SortedSet<String>> map = new LinkedHashMap<String, SortedSet<String>>(); Since you're doing a sort below, this shouldn't need to actually be a Linked HashMap. http://gwt-code-reviews.appspot.com/160801/diff/16001/17011#newcode276 dev/core/src/com/google/gwt/dev/cfg/BindingProperty.java:276: // Examine each orinial value in the set "original" http://gwt-code-reviews.appspot.com/160801/diff/16001/17011#newcode283 dev/core/src/com/google/gwt/dev/cfg/BindingProperty.java:283: // If so, merge the existing set into this one and update pointers Comment and code in opposite order. http://gwt-code-reviews.appspot.com/160801/diff/16001/17011#newcode299 dev/core/src/com/google/gwt/dev/cfg/BindingProperty.java:299: return o1.toString().compareTo(o2.toString()); String s1 = o1.toString(); String s2 = o2.toString(); assert !s1.equals(s2); // asserts no two sets are the same return s1.compareTo(s2); http://gwt-code-reviews.appspot.com/160801/diff/16001/17012 File dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17012#newcode407 dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:407: public void setCollapseAllProperties(boolean collapse) { I know it's a break with tradition, but this (and really all the other mutators) ought to be package-protected. Wanna break the old bad trend? http://gwt-code-reviews.appspot.com/160801/diff/16001/17013 File dev/core/src/com/google/gwt/dev/cfg/ModuleDefSchema.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17013#newcode1034 dev/core/src/com/google/gwt/dev/cfg/ModuleDefSchema.java:1034: private static class PropertyValueGlob { This should sort down with the other new classes. http://gwt-code-reviews.appspot.com/160801/diff/16001/17015 File dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17015#newcode240 dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java:240: ResolveRebinds.exec(jprogram, permutation); I admit this is nitpicky, but we've tried to avoid polluting the compiler core with higher-level constructs. Looking over the ResolveRebinds changes, it seems like you could just pass in a Map<String,String>[] and the logic would be slightly simpler. http://gwt-code-reviews.appspot.com/160801/diff/16001/17017 File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17017#newcode1476 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1476: * function gwtOnLoad(errFn, modName, modBase){ Please update doc. http://gwt-code-reviews.appspot.com/160801/diff/16001/17018 File dev/core/src/com/google/gwt/dev/jjs/impl/ResolveRebinds.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17018#newcode204 dev/core/src/com/google/gwt/dev/jjs/impl/ResolveRebinds.java:204: list.add(permutationId(oracle)); Shouldn't this always just be the index of the current oracle within permutation.getPropertyOracles()? In order words if you iterate with an int explicitly, I think you don't need the N^2 lookup here. http://gwt-code-reviews.appspot.com/160801/diff/16001/17018#newcode208 dev/core/src/com/google/gwt/dev/jjs/impl/ResolveRebinds.java:208: JClassType fallbackType = null; Why do we need a fall-back expression? Shouldn't it always work out correctly? If it's a code-size optimization to do so, maybe we should just call it "mostUsed" and note it as an optimization. http://gwt-code-reviews.appspot.com/160801/diff/16001/17018#newcode224 dev/core/src/com/google/gwt/dev/jjs/impl/ResolveRebinds.java:224: program.getTypeJavaLangObject(), false, true, true, false, false); This ought to be the non-null java.lang.Object(). (This is the same type returned by JGwtCreate.getType()). http://gwt-code-reviews.appspot.com/160801/diff/16001/17018#newcode247 dev/core/src/com/google/gwt/dev/jjs/impl/ResolveRebinds.java:247: Collections.sort(permutations); Should already be sorted. http://gwt-code-reviews.appspot.com/160801/diff/16001/17020 File dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/CollapsedPropertyHolder.java (right): http://gwt-code-reviews.appspot.com/160801/diff/16001/17020#newcode22 dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/CollapsedPropertyHolder.java:22: final class CollapsedPropertyHolder { Seems kinda silly to have to run a static initializer every time just for the sake of checking an assertion. What about this formulation? final class CollapsedPropertyHolder { public static int getPermutationId() { assert getPermutationIdOrNegOne() != -1 : "The bootstrap linker did not " + "provide a soft permutation id to the gwtOnLoad function"; return getPermutationId0(); } private static native int getPermutationId0() /*-{ return $softPermutationId; }-*/; private static native int getPermutationIdOrNegOne() /*-{ return ($softPermutationId == undefined) ? -1 : $softPermutationId; }-*/; } http://gwt-code-reviews.appspot.com/160801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors To unsubscribe from this group, send email to google-web-toolkit-contributors+unsubscribegooglegroups.com or reply to this email with the words "REMOVE ME" as the subject.
