[gwt-contrib] Re: Adds a -strict option to the GWT compiler. If this option is specified, (issue853801)

[spoon]

http://gwt-code-reviews.appspot.com/853801/diff/4001/5008
File user/test/com/google/gwt/dev/StrictModeTest.java (right):

http://gwt-code-reviews.appspot.com/853801/diff/4001/5008#newcode45
user/test/com/google/gwt/dev/StrictModeTest.java:45: private File
outDir;
The comment needs updating.

I started to drop support for the temp dir, but it's still needed for
the successful compiles. The output will go somewhere.

http://gwt-code-reviews.appspot.com/853801/diff/4001/5008#newcode54
user/test/com/google/gwt/dev/StrictModeTest.java:54: fail(Should have
compiled successfully);
Yes, I'll change it that way.

http://gwt-code-reviews.appspot.com/853801/diff/4001/5008#newcode66
user/test/com/google/gwt/dev/StrictModeTest.java:66: } catch
(UnableToCompleteException e) {
Sounds good.

http://gwt-code-reviews.appspot.com/853801/diff/4001/5008#newcode167
user/test/com/google/gwt/dev/StrictModeTest.java:167: private void
precompile(boolean good) throws UnableToCompleteException {
Nice trick! Booleans are hard to read because it's impossible to
remember what true and false refer to.

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

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

[gwt-contrib] Re: Adds a -strict option to the GWT compiler. If this option is specified, (issue853801)

[scottb]

http://gwt-code-reviews.appspot.com/853801/diff/4001/5008
File user/test/com/google/gwt/dev/StrictModeTest.java (right):

http://gwt-code-reviews.appspot.com/853801/diff/4001/5008#newcode45
user/test/com/google/gwt/dev/StrictModeTest.java:45: private File
outDir;
Strange, I don't see outDir referenced by the Options.  It's only being
used in setUp/tearDown.  Are you sure you need the out dir since you're
only calling Precompile, and never trying to link?

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

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

[gwt-contrib] Re: Adds a -strict option to the GWT compiler. If this option is specified, (issue853801)

[spoon]

http://gwt-code-reviews.appspot.com/853801/diff/4001/5008
File user/test/com/google/gwt/dev/StrictModeTest.java (right):

http://gwt-code-reviews.appspot.com/853801/diff/4001/5008#newcode45
user/test/com/google/gwt/dev/StrictModeTest.java:45: private File
outDir;
You're right.  I'll delete it. I had forgotten I changed it just to do a
precompile.

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

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

[gwt-contrib] Re: Adds a -strict option to the GWT compiler. If this option is specified, (issue853801)

[Scott Blum]

LGTM

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

[gwt-contrib] Re: Adds a -strict option to the GWT compiler. If this option is specified, (issue853801)

[spoon]

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

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

[gwt-contrib] Re: Adds a -strict option to the GWT compiler. If this option is specified, (issue853801)

[spoon]

Those two changes are made now. It's ready for another round of review.


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

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

[gwt-contrib] Re: Adds a -strict option to the GWT compiler. If this option is specified, (issue853801)

[scottb]

LGTM.  Just nits, no need to re-review if you decide to fix them.



http://gwt-code-reviews.appspot.com/853801/diff/4001/5008
File user/test/com/google/gwt/dev/StrictModeTest.java (right):

http://gwt-code-reviews.appspot.com/853801/diff/4001/5008#newcode45
user/test/com/google/gwt/dev/StrictModeTest.java:45: private File
outDir;
I think you can nuke this now, and setUp/tearDown, right?

http://gwt-code-reviews.appspot.com/853801/diff/4001/5008#newcode54
user/test/com/google/gwt/dev/StrictModeTest.java:54: fail(Should have
compiled successfully);
It might be clearer the way you wrote it, but you could also just
declare a 'throws' on the test method and not bother with the try/catch.

http://gwt-code-reviews.appspot.com/853801/diff/4001/5008#newcode66
user/test/com/google/gwt/dev/StrictModeTest.java:66: } catch
(UnableToCompleteException e) {
Total nit, but I've grown fond of naming the catch variable 'expected'
instead of adding the comment line. :)

http://gwt-code-reviews.appspot.com/853801/diff/4001/5008#newcode167
user/test/com/google/gwt/dev/StrictModeTest.java:167: private void
precompile(boolean good) throws UnableToCompleteException {
Instead of booleans, just have these two methods take (String
moduleName), and make GOOD and BAD module name constants.  The test code
should be a little more readable.

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

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

[gwt-contrib] Re: Adds a -strict option to the GWT compiler. If this option is specified, (issue853801)

[Scott Blum]

High-level comments:

- CompilationState doesn't really need the extra API, because you can always
just iterate through the returned collection looking for errors.  Arguably,
this would take a few extra cycles, but I've kinda been wanting to move the
error reporting out  CompilationState anyway and let the callers handle it,
which would eliminate the extra cost since we'd have to loop through
reporting errors anyhow.

- On the test class, it might be easier to just check a Good and Bad module
into dev/test instead of mucking around with temp dirs and class loaders.
 This is the pattern we use for TypeOracleTest.gwt.xml, and One.gwt.xml, for
example.

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