RFR [14] JDK-8225763 Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

Hello Lance,

On 10/07/19 2:25 AM, Lance Andersen wrote:
> ———
> @implSpec This method is a no-op if this compressor has already
> 886 * been previously closed,
>
> ————
>
> Please remove “already” in both the close() and end() methods.

Done.


>  I believe the preference is the @implSpec and its relatives are on
> their own line as in https://openjdk.java.net/jeps/8068562 and was
> done for @apiNote earlier

Done.


>
> Outside of the @ImplSpec, I am not sure the initial wording for end()
> and close() really need to differ:
>
> end():
>
> Closes the decompressor and discards any unprocessed input.
>
> close():
>
>     Releases resources held by this decompressor and discards any
>     unprocessed input.This method should be called when the
>     decompressor is no longer needed
>
>
I have now updated the javadoc of end() to be closer to the javadoc of
close().


>
>> - The javadoc of end() method in both these classes has been updated to
>> encourage the use of close() method instead of this one. It now also has
>> a @implSpec which states that it's a no-op if called more than once.
>>
>> In addition, this javadoc has also been updated to replace the
>> "undefined behaviour" statement with a mention that the usage of the
>> Inflater/Deflater instance after a call to end() may throw an exception
>> in those subsequent usages. Please note that, there's no such explicit
>> mention in the javadoc of the (newly added) close() method because IMO,
>> it isn't needed for close() since I think it's kind of implied that a
>> closed resource can no longer be used for further operations.
>
> We need to be specific in close()  also for clarity

I haven't updated this in the latest webrev version and will wait for us
to come to a decision on how we word it for end().


>>
>>
>> - TotalInOut.java test has been updated to use the new
>> try-with-resources construct for the inflater and deflater it uses.
>
> Please update @biug to include the bug number

Done.


>>
>> - A couple of new (testng) test classes have been added to test various
>> scenarios where close() and end() method get called. These test mostly
>> focus on ensuring that the close() and end(), either implicitly or
>> explicitly, get called the right number of times on the subclasses of
>> Inflater/Deflater.
>
> Overall they look OK.  In your tests, you are testing the number of
> calls for the sub-classes not for Deflate/Inflate so I would either
> update your comments to clarify that or pull them into their own test
> methods
>
I did not understand this. Did you mean I should update the @summary
part of these tests or was it the javadoc on these test methods?

The latest webrev with the above noted changes is available at
http://cr.openjdk.java.net/~jpai/webrev/8225763/3/webrev/

-Jaikiran