[gwt-contrib] Re: Bug triggered in SourceFileCompilationUnit.asCachedCompilation() (issue1461803)
http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java
File dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java
(right):
http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java#newcode139
dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java:139:
InputStream in = resource.openContents();
This Util.copy isn't needed to be inside the try catch.
http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/src/com/google/gwt/dev/javac/JavaSourceParser.java
File dev/core/src/com/google/gwt/dev/javac/JavaSourceParser.java
(right):
http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/src/com/google/gwt/dev/javac/JavaSourceParser.java#newcode294
dev/core/src/com/google/gwt/dev/javac/JavaSourceParser.java:294:
InputStream stream = classSource.openContents();
Util.readStreamAsString doesn't need to be inside try/catch
http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/src/com/google/gwt/dev/resource/Resource.java
File dev/core/src/com/google/gwt/dev/resource/Resource.java (right):
http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/src/com/google/gwt/dev/resource/Resource.java#newcode84
dev/core/src/com/google/gwt/dev/resource/Resource.java:84: /**
This comment is no longer valid, no? It can't return null anymore. But
it gives me pause, since it mentions the resource might be invalildated
intentionally by its containing resource. Do we know what that's about?
Since much of this change is to treat an IOException as a
RuntimeException, etc. (not that the current behavior is essentially
resulting in that anyway)...
http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/test/com/google/gwt/dev/resource/impl/FileResourceTest.java
File
dev/core/test/com/google/gwt/dev/resource/impl/FileResourceTest.java
(right):
http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/test/com/google/gwt/dev/resource/impl/FileResourceTest.java#newcode75
dev/core/test/com/google/gwt/dev/resource/impl/FileResourceTest.java:75:
Not sure what this comment is supposed to be saying (and I relealize
it's not your mod). 10 Gold stars if you can disambiguate!
http://gwt-code-reviews.appspot.com/1461803/diff/6002/user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java
File user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java
(right):
http://gwt-code-reviews.appspot.com/1461803/diff/6002/user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java#newcode204
user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java:204: }
catch (IOException iex) {
This changes the behavior, from being an NPE to essentially a
UnableToCompleteException. I assume that's ok.
http://gwt-code-reviews.appspot.com/1461803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Bug triggered in SourceFileCompilationUnit.asCachedCompilation() (issue1461803)
I've seen NPEs in other scenarios, so it seemed worth it to go ahead and
have Resource.openContents let theIOException through so it could be
logged in the most appropriate way.
http://gwt-code-reviews.appspot.com/1461803/diff/4003/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/1461803/diff/4003/dev/core/src/com/google/gwt/dev/javac/SourceFileCompilationUnit.java#newcode60
dev/core/src/com/google/gwt/dev/javac/SourceFileCompilationUnit.java:60:
if (sourceToken < 0) {
I'm wondering why we don't just throw an exception from openContents().
Returning null is obscuring a real problem, I don't see any cases where
a caller catches the null ptr and doe something smart. Do you know how
hard I had to work to narrow this down on a machine that didn't have a
development env?
On 2011/06/25 16:14:33, jbrosenberg wrote:
I think 'in' can end up being null, without an exception being thrown
(e.g. in
File/ZipResource.openConnection(), it catch any IOException and return
null).
Shouldn't this handle that possibility a bit more gracefully? The
assert for
(in != null) in transferFromStream would seemingly blow up in that
case
http://gwt-code-reviews.appspot.com/1461803/diff/4003/dev/core/src/com/google/gwt/dev/util/DiskCache.java
File dev/core/src/com/google/gwt/dev/util/DiskCache.java (right):
http://gwt-code-reviews.appspot.com/1461803/diff/4003/dev/core/src/com/google/gwt/dev/util/DiskCache.java#newcode139
dev/core/src/com/google/gwt/dev/util/DiskCache.java:139: * Write the
rest of the data in an input stream to disk. Note: this method
On 2011/06/25 16:14:33, jbrosenberg wrote:
s/close the in InputStream/close the InputStream
I meant for 'in' to be the parameter name, I'll make it more clear.
http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/src/com/google/gwt/core/ext/linker/impl/StandardPublicResource.java
File
dev/core/src/com/google/gwt/core/ext/linker/impl/StandardPublicResource.java
(right):
http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/src/com/google/gwt/core/ext/linker/impl/StandardPublicResource.java#newcode82
dev/core/src/com/google/gwt/core/ext/linker/impl/StandardPublicResource.java:82:
public InputStream getContents(TreeLogger logger)
I looked for references to this method, and none of them were explicitly
checking for null. They would have failed later with an NPE, hiding the
real cause.
http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/test/com/google/gwt/dev/resource/impl/FileResourceTest.java
File
dev/core/test/com/google/gwt/dev/resource/impl/FileResourceTest.java
(right):
http://gwt-code-reviews.appspot.com/1461803/diff/6002/dev/core/test/com/google/gwt/dev/resource/impl/FileResourceTest.java#newcode77
dev/core/test/com/google/gwt/dev/resource/impl/FileResourceTest.java:77:
try {
This is the only place in the codebase I could find where a null return
value was checked and used intentionally.
http://gwt-code-reviews.appspot.com/1461803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Enable generator result caching, by default. (issue1467808)
LGTM http://gwt-code-reviews.appspot.com/1467808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Enable generator result caching, by default. (issue1467808)
http://gwt-code-reviews.appspot.com/1467808/diff/1/dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerEnableGeneratorResultCaching.java
File
dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerEnableGeneratorResultCaching.java
(right):
http://gwt-code-reviews.appspot.com/1467808/diff/1/dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerEnableGeneratorResultCaching.java#newcode48
dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerEnableGeneratorResultCaching.java:48:
public boolean setFlag() {
On 2011/06/25 15:59:59, jbrosenberg wrote:
Well, I looked into that. Since there's no logger passed into this
class, it's
kinda difficult for it to generate warning output.
maybe too much work, but You could have it change the value of the an
option from false to true, then check the option in the main body
flag... if its true, then print the warning, if its false, go on about
your business silently.
http://gwt-code-reviews.appspot.com/1467808/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Bug triggered in SourceFileCompilationUnit.asCachedCompilation() (issue1461803)
http://gwt-code-reviews.appspot.com/1461803/diff/4003/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/1461803/diff/4003/dev/core/src/com/google/gwt/dev/javac/SourceFileCompilationUnit.java#newcode60
dev/core/src/com/google/gwt/dev/javac/SourceFileCompilationUnit.java:60:
if (sourceToken < 0) {
I think 'in' can end up being null, without an exception being thrown
(e.g. in File/ZipResource.openConnection(), it catch any IOException and
return null). Shouldn't this handle that possibility a bit more
gracefully? The assert for (in != null) in transferFromStream would
seemingly blow up in that case
http://gwt-code-reviews.appspot.com/1461803/diff/4003/dev/core/src/com/google/gwt/dev/util/DiskCache.java
File dev/core/src/com/google/gwt/dev/util/DiskCache.java (right):
http://gwt-code-reviews.appspot.com/1461803/diff/4003/dev/core/src/com/google/gwt/dev/util/DiskCache.java#newcode139
dev/core/src/com/google/gwt/dev/util/DiskCache.java:139: * Write the
rest of the data in an input stream to disk. Note: this method
s/close the in InputStream/close the InputStream
http://gwt-code-reviews.appspot.com/1461803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Enable generator result caching, by default. (issue1467808)
http://gwt-code-reviews.appspot.com/1467808/diff/1/dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerEnableGeneratorResultCaching.java
File
dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerEnableGeneratorResultCaching.java
(right):
http://gwt-code-reviews.appspot.com/1467808/diff/1/dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerEnableGeneratorResultCaching.java#newcode48
dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerEnableGeneratorResultCaching.java:48:
public boolean setFlag() {
Well, I looked into that. Since there's no logger passed into this
class, it's kinda difficult for it to generate warning output.
http://gwt-code-reviews.appspot.com/1467808/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Enable generator result caching, by default. (issue1467808)
http://gwt-code-reviews.appspot.com/1467808/diff/1/dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerEnableGeneratorResultCaching.java
File
dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerEnableGeneratorResultCaching.java
(right):
http://gwt-code-reviews.appspot.com/1467808/diff/1/dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerEnableGeneratorResultCaching.java#newcode48
dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerEnableGeneratorResultCaching.java:48:
public boolean setFlag() {
Is there some way we can log an ugly advisory warning that this flag has
no effect so people will actually remove it?
http://gwt-code-reviews.appspot.com/1467808/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
