First set of comments
http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/CompileModule.java File dev/core/src/com/google/gwt/dev/CompileModule.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode122 dev/core/src/com/google/gwt/dev/CompileModule.java:122: public static void main(String[] args) { This code is almost identical to Compile.main as well as others. Can we refactor it? It's especially disturbing to see the 4-line comment about System.exit duplicated in 11 different places in our codebase. http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode192 dev/core/src/com/google/gwt/dev/CompileModule.java:192: if (archive.getTopModuleName().equals(moduleToCompile)) { Why are we not loading up CompilationUnits for the module we're compiling? For example, if the module has 100 source files, but only 1 is stale, won't the caching we already have work correctly to keep the 99 non-stale units and throw away the single stale unit? Also, since we're rolling up dependent modules, isn't this going to cause a miss on all those dependent modules included in that archive? http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java File dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java#newcode94 dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:94: private final Set<URL> archiveURLs = new LinkedHashSet<URL>(); I know this class is mostly comment free, but it would be great to have a comment here explaining what archiveURLs are. http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java#newcode94 dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:94: private final Set<URL> archiveURLs = new LinkedHashSet<URL>(); Turns out you don't want to use Set<URL>, because URL.equals() has really bad performance behavior: http://michaelscharf.blogspot.com/2006/11/javaneturlequals-and-hashcode-make.html You can use a different datatype like URI or String, or change this to a Collection that doesn't use equals by default.(Though that can be dangerous, because someone can accidentally use a method like contains() that will use equals). http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java#newcode141 dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:141: public void addCompilationUnitArchiveURL(URL url) { Should this be package access? http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java#newcode310 dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:310: public URL[] getAllCompilationUnitArchives() { Why not just use the Set? If you're being defensive, why not Collections.unmodifiableSet()? http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java File dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java (left): http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java#oldcode114 dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java:114: * {@link #loadFromClassPath(logger, moduleName, false)}. This should be {@link #loadFromClassPath(TreeLogger, String, boolean) loadFromClassPath(logger, moduleName, false)} http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java File dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java#newcode71 dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java:71: public static final String COMPILATION_UNIT_ARCHIVE_SUFFIX = ".gwt"; .gwt seems a bit generic. Some ideas: .gar (GWT archive), .gwt-mar (GWT module archive), .gwt-module-cache (self explanatory) http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java#newcode267 dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java:267: String compilationUnitArchiveName = slashedModuleName + ModuleDefLoader.COMPILATION_UNIT_ARCHIVE_SUFFIX; It seems a bit strange to me that all dependent modules are being rolled up. Doing that seems like you're going to end up with copies of dependencies in different modules. http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java File dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/6001/dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java#newcode74 dev/core/src/com/google/gwt/dev/javac/CompilationUnitArchive.java:74: private transient SortedMap<String, CompilationUnit> units; If you only need sorting for deterministic output, should you just make this a HashMap and sort only on writeObject? http://gwt-code-reviews.appspot.com/1448808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
