On Thu, 7 Nov 2024 14:16:36 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 with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains nine additional > commits since the last revision: > > - update tests to match the new specification > - Stuart's review - update the close() and end() expectations > - Stuart's review - improve class level javadoc > - merge latest from master branch > - merge latest from master branch > - Chen's suggestion - improve code comment > - convert the tests to junit > - fix whitespace > - 8225763: Inflater and Deflater should implement AutoCloseable src/java.base/share/classes/java/util/zip/Deflater.java line 902: > 900: */ > 901: @Override > 902: public void close() { Can/should this method be final? The real/original cleanup method is `end` and if both `close()` and `end()` are overriddable, there may be some confusion about which to implement. src/java.base/share/classes/java/util/zip/Inflater.java line 719: > 717: */ > 718: @Override > 719: public void close() { Ditto, should this be `final` to be clear about what should be overridden to perform cleanup. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1832777080 PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1832787363
