On Thu, 13 Jun 2024 14:09:16 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> src/java.base/share/classes/java/util/zip/Deflater.java line 904:
>>
>>> 902: public void close() {
>>> 903: synchronized (zsRef) {
>>> 904: // check if already closed
>>
>> Should we comment `// in case subclasses override the closed check in end()`
>> instead? Otherwise the duplicate comments and checks are confusing.
>
> Hello Chen, we want the close() to be idempotent irrespective of whether or
> not end() is overridden. That check in close() and the code comment on that
> line is to indicate that. If the comment is adding to confusion, I can remove
> it - I don't think it's a must to have it to understand the check.
I mean, we have updated the default implementation of end() to be idempotent.
It now just appears confusing to readers that close() **seems to** perform
redundant checks for idempotence until they realize subclasses can override
end() to make it non-idempotent.
My suggestion is that our comment should say why we have this check here even
though there's already an identical check in end().
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1638471725
