http://gwt-code-reviews.appspot.com/1448808/diff/1/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/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode147 dev/core/src/com/google/gwt/dev/CompileModule.java:147: static UnableToCompleteException logAndTranslateException(TreeLogger logger, Throwable e) { On 2011/06/01 20:16:40, jbrosenberg wrote:
This code seems to be duplicated several places, in
JavaToJavaScriptCompiler and
GwtAstBuilder. Can it be consolidated? Maybe as part of LoggerUtils
class,
etc.?
Done. Moved to CompilationProblemReporter http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode194 dev/core/src/com/google/gwt/dev/CompileModule.java:194: On 2011/06/01 20:16:40, jbrosenberg wrote:
Add an overview comment for this method?
Done. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode195 dev/core/src/com/google/gwt/dev/CompileModule.java:195: public boolean run(final TreeLogger logger) { On 2011/06/01 20:16:40, jbrosenberg wrote:
It seems like the order that modules are processed is important here.
Should it
be sub-modules first, then parent modules, etc. Otherwise a parent
module will
end up including the units of a sub-module, and then if a sub-module
is
processed, it will have to again write a compiled sub-module file with
the same
units again. I guess, I'm not sure why units from a sub-module can be
included
in a parent module, shouldn't each sub-module contain only it's units
(with
possible updating to these if needed?). It seems like it is only
writing a
final top-level compiled module file, which can contain all units that
haven't
been found in any precompiled modules so far.
For instance, we wouldn't want the units for a widget library to be
separately
included in each application module, instead we want to keep all the
library's
units in it's own pre-compiled cache.
A followon patch might analyze all the .gwt.xml files in a tree and order them properly so that they can be compiled and written out in a sane fashion. But that is a pretty sticky problem that I decided to avoid for now. The main issue is that modules have circular dependencies, and many modules don't properly include all their dependencies. We'll have to sort that out in gwt-user to make it all work. In the mean time, since User.gwt.xml includes the lion's share of the code in gwt-user.jar, I feel that a pre-built Core.gwt.xml and User.gwt.xml are going to serve most users well, and that's what I've included in the ant build. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode204 dev/core/src/com/google/gwt/dev/CompileModule.java:204: On 2011/06/01 20:16:40, jbrosenberg wrote:
Might be useful to add module name to the speed tracer event.
Done. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode206 dev/core/src/com/google/gwt/dev/CompileModule.java:206: SpeedTracerLogger.start(CompilerEventType.LOAD_PRECOMPILED); On 2011/06/01 20:16:40, jbrosenberg wrote:
compiledModuleURLs?
Done. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode219 dev/core/src/com/google/gwt/dev/CompileModule.java:219: if (compiledModule.getName().equals(moduleToCompile)) { On 2011/06/01 20:16:40, jbrosenberg wrote:
Don't we risk throwing away already compiled units here? It seems
like below,
for 'moduleToCompile', a fresh CompiledGwtModule is created, and
everything
compiled fresh?
Paranoia, I guess. I'd have to come up with a better strategy for determining which compiled units should be written out if I accepted the previous compiled version of this module. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode233 dev/core/src/com/google/gwt/dev/CompileModule.java:233: } On 2011/06/01 20:16:40, jbrosenberg wrote:
put in a finally block, in case an unchecked exception happens.
Done. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode255 dev/core/src/com/google/gwt/dev/CompileModule.java:255: if (!outputDir.isDirectory() && !outputDir.mkdirs()) { On 2011/06/01 20:16:40, jbrosenberg wrote:
don't we also need to bail altogether, in this case?
Done. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode271 dev/core/src/com/google/gwt/dev/CompileModule.java:271: + ex); needs to bail here too. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompiledGwtModule.java File dev/core/src/com/google/gwt/dev/CompiledGwtModule.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompiledGwtModule.java#newcode36 dev/core/src/com/google/gwt/dev/CompiledGwtModule.java:36: On 2011/06/01 20:16:40, jbrosenberg wrote:
Since this might not contain a complete set of units for a module, and
in fact
can contain units of nested modules, perhaps this should be called
something
like:
ModuleCompiledUnitCache (or some such)
I would like for the CompileModule to eventually get smarter, but maybe the name does mis-represent. How about CompilationUnitArchive, as an analog to a Java Archive jar) http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/Precompile.java File dev/core/src/com/google/gwt/dev/Precompile.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/Precompile.java#newcode349 dev/core/src/com/google/gwt/dev/Precompile.java:349: SpeedTracerLogger.start(CompilerEventType.LOAD_PRECOMPILED); On 2011/06/01 20:16:40, jbrosenberg wrote:
compiledModuleURLs
Done. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/Precompile.java#newcode368 dev/core/src/com/google/gwt/dev/Precompile.java:368: } On 2011/06/01 20:16:40, jbrosenberg wrote:
put in finally block, in case of unchecked exception
Done. http://gwt-code-reviews.appspot.com/1448808/diff/1/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/1/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java#newcode91 dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:91: On 2011/06/01 20:16:40, jbrosenberg wrote:
This is essentially a list of URL's for nested modules which are
compiled.
Consider changing name to something like:
compiledNestedGwtModuleURLs (well I guess that's ugly).
Point is, I wasn't sure at first what this (and the associated methods
below)
were referring to. So maybe something slightly clearer?
These are URL's, representing module files, for compiled nested
modules....:) Done. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java#newcode138 dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:138: On 2011/06/01 20:16:40, jbrosenberg wrote:
Change param name to 'compiledGwtModule'?
renamed it all... let me know what you think. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode381 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:381: On 2011/06/01 20:16:40, jbrosenberg wrote:
DELETE HIM
Done. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode484 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:484: public static void addCompiledGwtModule(TreeLogger logger, CompiledGwtModule module) { On 2011/06/01 20:16:40, jbrosenberg wrote:
instance.unitCache?
Done. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java File dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java#newcode52 dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java:52: CompiledGwtModule m2 = CompiledGwtModule.createFromFile(tmp); On 2011/06/01 20:16:40, jbrosenberg wrote:
s/m2/m/
m.getUnits().size() now m2.getUnits().size() http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java#newcode56 dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java:56: assertEquals(m2.findUnit(unit3.getResourcePath()).getTypeName(), MOCK_TYPE_3); On 2011/06/01 20:16:40, jbrosenberg wrote:
Maybe also check that you read back what you put, so compare m equals
m2 in each
aspect.
I compared Source and resource location, since everything else in mock compilation unit is pretty much bogus. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java#newcode59 dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java:59: /** On 2011/06/01 20:16:40, jbrosenberg wrote:
s/are insist/insist
Done. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java#newcode64 dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java:64: int numMockTypes = 100; On 2011/06/01 20:16:40, jbrosenberg wrote:
don't really need an array to store mockTypes, can just use an
iteration
variable each time.
Done. http://gwt-code-reviews.appspot.com/1448808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
