John, here's what I have so far, but I gotta run. I didn't fine-tooth-comb JSORestrictionsChecker, but I'm sure it's good enough to go it.
Some of my other concerns I would like to see resolved. With a commit this big I think it makes sense to break it up to the extent possible. I also did not really look too much at the stuff in dev.javac, dev.resource, and TypeOracle, since I know most of that is big rewrites. I was largely looking at the impact on the rest of the system. http://gwt-code-reviews.appspot.com/51826/diff/1/2 File branch-info.txt (right): http://gwt-code-reviews.appspot.com/51826/diff/1/2#newcode1 Line 1: Created from tr...@r5326 Please remove before committing. :) http://gwt-code-reviews.appspot.com/51826/diff/1/77 File dev/core/src/com/google/gwt/dev/SwtHostedModeBase.java (right): http://gwt-code-reviews.appspot.com/51826/diff/1/77#newcode72 Line 72: TypeOracle typeOracle = moduleDef.getTypeOracle(logger, wantBinaries); I basically disagree with threading this through all the apis. If you want to leave an escape hatch in CompilationState itself, that's fine. I would recommend we follow the existing conventions and set a private static final boolean constant to System.getProperty("gwt.typeOracle.noUseClasses") != null http://gwt-code-reviews.appspot.com/51826/diff/1/76 File dev/core/src/com/google/gwt/dev/cfg/ModuleDefSchema.java (right): http://gwt-code-reviews.appspot.com/51826/diff/1/76#newcode58 Line 58: protected final String __binary_1_path = ""; Why are we going to all this trouble to make it configurable? Shouldn't it just always match the source packages? http://gwt-code-reviews.appspot.com/51826/diff/1/76#newcode612 Line 612: logger.log(TreeLogger.WARN, "Non-relative public package: " 'public' is wrong +2 more instances http://gwt-code-reviews.appspot.com/51826/diff/1/76#newcode834 Line 834: private final class IncludeExcludeSchema extends Schema { Don't do that. +2 more occurrences in this file. http://gwt-code-reviews.appspot.com/51826/diff/1/76#newcode1248 Line 1248: bodySchema.addBinaryPackage(modulePackageAsPath, "", Empty.STRINGS, Why wouldn't the default be 'client' to match the source path? http://gwt-code-reviews.appspot.com/51826/diff/1/65 File dev/core/src/com/google/gwt/dev/javac/JSORestrictionsChecker.java (left): http://gwt-code-reviews.appspot.com/51826/diff/1/65#oldcode344 Line 344: GWTProblem.recordInCud(node, cud, error, new InstalledHelpInfo( Did you nuke this checker's ability to produce a link to the help doc? http://gwt-code-reviews.appspot.com/51826/diff/1/65 File dev/core/src/com/google/gwt/dev/javac/JSORestrictionsChecker.java (right): http://gwt-code-reviews.appspot.com/51826/diff/1/65#newcode203 Line 203: private void fail() { Just for this visitor, it seems to make the most sense for this class to be static and just throw an abort out the top if you fail, catch and add the error there. http://gwt-code-reviews.appspot.com/51826/diff/1/65#newcode322 Line 322: * If it is a non-static inner class (which we will give an error for If we hit a non-static inner class, let's just abort and stop processing. Wouldn't that make EmptyConstructorChecker simpler? Also, the implementation might be somewhat easier to follow if somehow we could encode the correct state transitions into the enum members. But maybe that's farfetched. http://gwt-code-reviews.appspot.com/51826/diff/1/65#newcode330 Line 330: protected enum CtorState { Can we make this a static inner class of EmptyConstructorChecker? http://gwt-code-reviews.appspot.com/51826/diff/1/71 File dev/core/src/com/google/gwt/dev/shell/CompilingClassLoader.java (right): http://gwt-code-reviews.appspot.com/51826/diff/1/71#newcode1098 Line 1098: assert unit != null; Unrelated change? http://gwt-code-reviews.appspot.com/51826/diff/1/70 File dev/core/src/com/google/gwt/dev/shell/ShellModuleSpaceHost.java (right): http://gwt-code-reviews.appspot.com/51826/diff/1/70#newcode1 Line 1: /* This file gets a straight revert if we unthread the system property to disable class reads. http://gwt-code-reviews.appspot.com/51826/diff/1/69 File dev/core/src/com/google/gwt/dev/shell/jetty/JettyLauncher.java (right): http://gwt-code-reviews.appspot.com/51826/diff/1/69#newcode1 Line 1: /* I'm sure this is a great patch, but it doesn't belong with IHM, right? http://gwt-code-reviews.appspot.com/51826/diff/1/80 File dev/core/src/com/google/gwt/dev/util/Name.java (right): http://gwt-code-reviews.appspot.com/51826/diff/1/80#newcode1 Line 1: /* What would be kind of awesome is to get this class, plus the changes to callers of this class that aren't related to IHM (CCL, for example) in first as an initial commit. There are actually several files with no IHM-related changes at all, they're only being modded for the sake of using this class. http://gwt-code-reviews.appspot.com/51826/diff/1/80#newcode213 Line 213: public static String lookupBinaryName(String sourceName, This method really doesn't belong in this class. Actually, I don't think it's called anywhere at all. http://gwt-code-reviews.appspot.com/51826/diff/1/78 File dev/core/src/com/google/gwt/dev/util/UnitTestTreeLogger.java (right): http://gwt-code-reviews.appspot.com/51826/diff/1/78#newcode186 Line 186: if (expectedEntries.size() != actualEntries.size()) { Seems unrelated to IHM. http://gwt-code-reviews.appspot.com/51826/diff/1/3 File dev/linux/src/com/google/gwt/dev/shell/moz/ModuleSpaceMoz.java (right): http://gwt-code-reviews.appspot.com/51826/diff/1/3#newcode128 Line 128: * @param file the file name containing the JSNI Unrelated. http://gwt-code-reviews.appspot.com/51826/diff/1/82 File user/src/com/google/gwt/junit/JUnitShell.java (right): http://gwt-code-reviews.appspot.com/51826/diff/1/82#newcode423 Line 423: // TODO(jat): how to make JUnitShell use binaries where appropriate? What does this mean? Seems like it should just use binaries by default, which would mean this comment can get removed. http://gwt-code-reviews.appspot.com/51826/diff/1/83 File user/super/com/google/gwt/emul/java/util/Date.java (right): http://gwt-code-reviews.appspot.com/51826/diff/1/83#newcode33 Line 33: Unrelated. http://gwt-code-reviews.appspot.com/51826 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
