Inputs on patch for JDK-8225763? Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

Hello Lance,

On 08/07/19 11:16 PM, Lance Andersen wrote:
> Hi Jaikiran,
>
> Thank you for your efforts here.
>
>> On Jul 6, 2019, at 9:59 AM, Jaikiran Pai <jai.forums2013 at gmail.com
>> <mailto:jai.forums2013 at gmail.com>> wrote:
>>
>>>
>>>  - Idempotency
>>>
>>> Yes, it looks to me like Inflater.end() and Deflater.end()
>>> implementations are idempotent. As you point out, overrides by
>>> subclasses might not be. We should be clear when we're talking about
>>> the specific implementatations of the end() methods in the Deflater
>>> and Inflater classes, or whether we're talking about the contracts
>>> defined by these method specifications on these classes and subtypes.
>>>
>>> The behavior of an implementation in a class can be specified with
>>> @implSpec without imposing this on the contract of subclasses. This is
>>> useful for subclasses that need to know the exact behavior of calling
>>> super.end(), and also for callers who know they have an instance of a
>>> particular class and not a subclass.
>>>
>>> The upshot is that while we might not have the end() method's contract
>>> specify idempotency, we can certainly say so in an @implSpec, if doing
>>> this will help. I'm not sure we'll actually need it in this case, but
>>> let's keep it in mind.
>>
>> Thank you. Although not for end(), I have now used this @implSpec in the
>> first version of my patch[1].
>
> This should be done for end() as well to set expectations.  While
> close() is the preferred way to go moving forward, end() is not going
> anywhere and still needs to be a first class-citizen WRT documentation.

Done. I have added the @implSpec for end() too in the new updated
revision of the webrev. I have opened a separate RFR thread containing
that webrev.


>>>
>>> **
>>>
>>> If you think the issues are settled enough, maybe it's time for you to
>>> take a stab at a patch. 
>>
>> The initial version of my patch is now ready at [1]. Here's a high level
>> summary of what's contained in this patch:
>
> Please start a review request thread so it is easier to follow.  Once
> you do that, I will reply to it..

Done - I have now created a new RFR thread for this here
http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-July/061229.html


>
> Also, reminder to update copyright dates.

The updated webrev in the RFR thread contains this update.


>
> For the tests, it would be best to add more comments.  Out of
> curiosity, was there a reason you chose not to use TestNG vs having
> just a main which invokes each test?

No specific reason. I'm still relatively new to the JDK codebase and
don't know when to use testng as against the regular main() driven
tests. I have now cleaned up the tests and converted them to testng in
the new webrev that I proposed.

-Jaikiran