On 2/24/11 12:36 AM, Xueming Shen wrote:
Stuart,
Here are my comments on non-pack changes. I'm sure Kumar will look at those
pack files later.
GetMethodsReturnClones.java: ln#43 diamond conersion?
Oh yes, I could do that. How could I have forgotten about diamond? :-)
Available.java: it's not your change, but I believe we should do the
try-with_resources on the ZipFile zfile as well.
Good catch. There are a couple uses of ZipFile in this file that aren't
protected by TWR.
InfoZip.java.: ln#113/114 you probably don't need to separate out the "fis" in
try-with-resources, the FileInputStream
created should be closed by the ZipInputStream that it is embedded in.
LargeZip.java: ln#101-1-4/148-151, same as above, only need to close
ZipOut/InputStream
TestEmptyZip.java:
TestEmptyZip.java:
Comment.java: ln#60-62
CorruptedZipFiles.java ln#50
DeleteTempJar.java:
LargeZipFile.java.
ManyEntries.java
ReadZip.java.
ShortRead.java
only close the ZipIn/OutputStream
David answered this already (and I think we chatted briefly about this on the
phone call this morning too) and I think we agree that it's safer to "unroll"
the resources into separate variables to make sure they all get closed if
there's an exception during construction of one of the wrapper resources.
Thanks!
s'marks
-Sherman
On 2011-2-23 22:26, Stuart Marks wrote:
Hi Sherman, all,
Here's a webrev to convert code in the jar/zip implementation files and tests
to use the new Java 7 try-with-resources construct.
http://cr.openjdk.java.net/~smarks/reviews/7021582/webrev.0/
There are rather a lot of files, however, most of the changes are pretty
straightforward. A typical conversion changes code like this:
FileInputStream fis = new FileInputStream(filename);
// use fis
fis.close();
to this:
try (FileInputStream fis = new FileInputStream(filename)) {
// use fis
}
The majority of the conversions are like the above. However, there are a
several places where I had to rearrange things either in order to get them to
work at all, to improve robustness, or for general cleanup.
Some of these are marked with "TODO". I will of course remove these comments
before committing the changes. Where you see these comments, you might want
to give the code a bit more scrutiny.
Specific examples of things to look at follow:
* test/java/util/jar/JarEntry/GetMethodsReturnClones.java
I had to change the ordering of local variable declarations so that the
variable was visible where it's used. TWR introduces a nested block, so
obviously a local declared within will have to be moved outside in order to
be used outside. This occurs in several other places as well. In some cases
initialization order was changed. This shouldn't matter, though, since it's
things like opening a file vs. allocating an array.
* src/share/classes/com/sun/java/util/jar/pack/Driver.java
I narrowed the scope of the open resource. No sense keeping it open any
longer than necessary. This occurs in several other places as well.
* src/share/classes/com/sun/java/util/jar/pack/PackageReader.java
* src/share/classes/com/sun/java/util/jar/pack/PackageWriter.java
These changes rely on recent changes to TWR's handling of null resources.
Currently, TWR will avoid calling close if the resource is null. Joe checked
in this change just last week. Before that, a null resource would generate an
unavoidable NPE when it attempted to call close(). Handling of non-null
resources is unchanged.
I don't think the change to null handling is in a promoted build yet. Is it
OK to check in code that depends on it? All tests pass, but that just means
that the path where the resource is null isn't tested.
* src/share/classes/com/sun/java/util/jar/pack/PropMap.java
Narrowed the scope of catch IOException; should be OK since the code that was
migrated out cannot throw IOException.
* src/share/classes/com/sun/java/util/jar/pack/UnpackerImpl.java
This closes its input after successful processing. I changed this so that it
also closes its input if an exception is thrown.
* test/java/util/zip/LargeZip.java
I've "unrolled" a cascade of constructors into separate resource variables.
This also occurs in several other places. Basically code that used to look
like this:
ZipOutputStream zos = new ZipOutputStream(
new BufferedOutputStream(
new FileOutputStream(largeFile)));
// process zos
zos.close();
is converted to this:
try (FileOutputStream fos = new FileOutputStream(largeFile);
BufferedOutputStream bos = new BufferedOutputStream(fos);
ZipOutputStream zos = new ZipOutputStream(bos))
{
// process zos
}
I think this more robust, since it closes the FileOutputStream if an
exception occurs during the construction of one of the stacked streams, which
the original code did not handle. Since the wrapper streams will close their
underlying streams, this will result in redundant close() calls. However,
close() is supposed to be idempotent so this should be OK.
* test/java/util/zip/ZipFile/DeleteTempJar.java
I'm not sure if this properly handles an IOException caused by
HttpExchange.close(). Funny, the method isn't declared to throw IOE, but this
test did compile and pass.
* test/java/util/zip/ZipFile/LargeZipFile.java
I changed this to fail the test if close() were to throw IOE. I think this is
proper for test code.
* test/java/util/zip/ZipFile/ReadZip.java
I took the liberty of converting the file copying code to use the new
java.nio.file.Files utilities. Well, I'm really following Alan's lead here
since he's prompted me to do so in other places a couple times already. :-)
Thanks for reviewing!
s'marks
