Hi Jaikiran,
> On Jul 10, 2019, at 2:56 AM, Jaikiran Pai <jai.forums2...@gmail.com> wrote:
>
> 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
>> <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().
>
>
>
Its better but end() should include the wording
————
This method should be called when the compressor is no longer needed.
——————
It could read
This method or close() should be called when the compressor ……
Also, thinking about the following in end():
———
* Use of {@link #close()} is encouraged instead of this method.
———
This might be better served as an @apiNote so it stands out
>>
>>> - 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().
>
OK
>
>>>
>>>
>>> - 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?
>
>
Sorry if my comment was confusing. For example:
————
public void testCloseThenEnd() throws Exception {
119 final Deflater simpleDeflater = new Deflater();
120 closeThenEnd(simpleDeflater);
121
122 final OverrideClose overridenClose = new OverrideClose();
123 closeThenEnd(overridenClose);
124 // make sure close was called once
125 assertEquals(overridenClose.numTimesCloseCalled, 1, "Unexpected
number of calls to close()");
——————
As there is not an assert() for simpleDeflater, which is understandable, the
comment describing the test is not quite accurate should be slightly updated
> The latest webrev with the above noted changes is available at
> http://cr.openjdk.java.net/~jpai/webrev/8225763/3/webrev/
> <http://cr.openjdk.java.net/~jpai/webrev/8225763/3/webrev/>
> -Jaikiran
Best
Lance
<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen|
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
