On Wed, 8 Jan 2025 06:01:55 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review of this enhancement which proposes to enhance >> `java.util.zip.Deflater/Inflater` classes to now implement `AutoCloseable`? >> >> The actual work for this was done a few years back when we discussed the >> proposed approaches and then I raised a RFR. At that time I couldn't take >> this to completion. The current changes in this PR involve the >> implementation that was discussed at that time and also have implemented the >> review suggestions from that time. Here are those previous discussions and >> reviews: >> >> https://mail.openjdk.org/pipermail/core-libs-dev/2019-June/061079.html >> https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061177.html >> https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061229.html >> >> To summarize those discussions, we had concluded that: >> - `Deflater` and `Inflater` will implement the `AutoCloseable` interface >> - In the `close()` implementation we will invoke the `end()` method >> (`end()` can be potentially overridden by subclasses). >> - `close()` will be specified and implemented to be idempotent. Calling >> `close()` a second time or more will be a no-op. >> - Calling `end()` and then `close()`, although uncommon, will also support >> idempotency and that `close()` call will be a no-op. >> - However, calling `close()` and then `end()` will not guarantee idempotency >> and depending on the implementing subclass, the `end()` may throw an >> exception. >> >> New tests have been included as part of these changes and they continue to >> pass along with existing tests in tier1, tier2 and tier3. When I had >> originally added these new tests, I hadn't used junit. I can convert them to >> junit if that's preferable. >> >> I'll file a CSR shortly. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > fix javadoc tag ordering - "@throws" after "@return" src/java.base/share/classes/java/util/zip/Deflater.java line 52: > 50: * <p> > 51: * This class deflates sequences of bytes into ZLIB compressed data > format. > 52: * The input byte sequence is provided in either byte array or {@link > ByteBuffer}, We should probably fix this sentence to say "in either a byte array or ...". src/java.base/share/classes/java/util/zip/Deflater.java line 60: > 58: * {@code Deflater} by calling either the {@link #end()} or the {@link > #close()} method. > 59: * After the {@code Deflater} has been closed, subsequent calls to > several methods > 60: * of the {@code Deflater} will throw an {@link IllegalStateException}. This paragraph uses the definite article but there isn't a specific Deflater to speak of, and it's not a singleton. The first sentence of this paragraph might be better if re-worded "To release the resources used a Deflater, an application must close it by invoking its end() or close() method". src/java.base/share/classes/java/util/zip/Deflater.java line 66: > 64: * {@code try}-with-resources statement. The {@linkplain Deflater#close() > close() method} simply > 65: * calls {@code end()}. Subclasses should override {@linkplain #end()} to > clean up the > 66: * resources acquired by the subclass. I wonder if the note for subclasses should move to the end method as implSpec, it looks out of place in the class API note. src/java.base/share/classes/java/util/zip/Deflater.java line 892: > 890: * and discards any unprocessed input. > 891: * <p> > 892: * If this method is invoked multiple times, the second and > subsequent calls do nothing. I think this could be clearer if you replace this sentence with "If the Deflater is already closed then invoking this method has no effect." ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1914314823 PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1914408496 PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1914413749 PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1914410532
