Eric, I got halfway through this, and decided to hold off pending some
high level feedback.

Precompile is starting to get really unwieldy, there are now essentially
4 different code paths, and I'm not even sure all the options make sense
together. What happens if you -XdisableGeneratingOnShards with -perms?
I would almost lobby for breaking Precompile into two different entry
points.  Leave the old version behaviorally the same, but make a new
entry point that is intended to be called only after AnalyzeModule.  The
two can still share a lot of code, but this should clean up some of the
unweildyness, looking for marker files, and whether or not to blow away
the work dir.

It appears that are also some files in this patch which are nothing but
format changes.

Lemme know what you think about Precompile, and I'll continue looking at
the details.


http://gwt-code-reviews.appspot.com/1074801/diff/7001/8003
File dev/core/src/com/google/gwt/dev/AnalyzeModule.java (right):

http://gwt-code-reviews.appspot.com/1074801/diff/7001/8003#newcode47
dev/core/src/com/google/gwt/dev/AnalyzeModule.java:47: private final
PrecompileOptionsImpl precompileOptions = new PrecompileOptionsImpl();
Why does this both nest, and extend?  Pick one.

http://gwt-code-reviews.appspot.com/1074801/diff/7001/8003#newcode73
dev/core/src/com/google/gwt/dev/AnalyzeModule.java:73: private interface
AnalyzeModuleOptions extends Precompile.PrecompileOptions {
Maybe doc that the only reason for this class is to support future
options.  But honestly, I think I wouldn't bother creating new types for
this or ArgProcessor and just reused the Precompile ones directly until
some time where we actually do extend them.

http://gwt-code-reviews.appspot.com/1074801/diff/7001/8003#newcode114
dev/core/src/com/google/gwt/dev/AnalyzeModule.java:114: * Silently
returns <pre>null</pre> if the file is not found or another
Use <code>null</code> instead of pre.

http://gwt-code-reviews.appspot.com/1074801/diff/7001/8006
File dev/core/src/com/google/gwt/dev/Precompile.java (right):

http://gwt-code-reviews.appspot.com/1074801/diff/7001/8006#newcode702
dev/core/src/com/google/gwt/dev/Precompile.java:702: if (!new
File(compilerWorkDir, AnalyzeModule.OPTIONS_FILENAME).isFile()) {
How do you know whether this wasn't left from a previous compile?

http://gwt-code-reviews.appspot.com/1074801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

Reply via email to