LGTM, all nits. No need to re-review.
http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/SourceFileCompilationUnit.java File dev/core/src/com/google/gwt/dev/javac/SourceFileCompilationUnit.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/SourceFileCompilationUnit.java#newcode70 dev/core/src/com/google/gwt/dev/javac/SourceFileCompilationUnit.java:70: String sourceCode = Shared.readSource(sourceFile); SGTM! Good thinking. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java File dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode241 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:241: // These additional closes make the unit tests run reliably. Can remove this comment now (the fix to the reader thread closing makes this not true, but the extra closes are probably a good idea anyway in case any intermediary construction fails). http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode256 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:256: shutDownLatch.await(5000, TimeUnit.MILLISECONDS); Do we really need this? I'm thinking maybe we should just wait forever. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode318 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:318: } I think the other way was better, LinkedBlockingQueue never blocks on put/add. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode365 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:365: } Same in these two cases, add() is better. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode447 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:447: } Couldn't much of the above code be replaced with a call to getCacheFiles()? http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode449 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:449: File cacheFile = new File(cacheDirectory, filename); Per face to face discussion, we refactored to skip if cacheFile is the one we actually opened to write to. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode477 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:477: Utility.close(inputStream); Per face to face, closing FIS also in case constructing the OOS fails. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/UnitCache.java File dev/core/src/com/google/gwt/dev/javac/UnitCache.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/UnitCache.java#newcode52 dev/core/src/com/google/gwt/dev/javac/UnitCache.java:52: void remove(CompilationUnit unit); Not called from the unit test anywhere. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java File dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java#newcode23 dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java:23: * Unit test for {@link MemoryUnitCache} Checkstyle wants a period. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java File dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java#newcode25 dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java:25: */ copy header should go above imports http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java#newcode62 dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java:62: return "/mock/" + typeName; "/mock/" + Shared.toPath(typeName) (Will need to update unit tests.) http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java File dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java#newcode26 dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java:26: * Unit test for {@link PersistentUnitCache} period http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java#newcode33 dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java:33: deleteDirRecursive(lastCacheDir); Reuse Util.recursiveDelete(lastCacheDir, false) http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java#newcode182 dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java:182: private void testNumFiles(File unitCacheDir, int expected) { suggest: assertNumChildren(int expected, File) http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
