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
