Inputs on patch for JDK-8225763? Inflater and Deflater should implement AutoCloseable
jai.forums2013 at gmail.com
Tue Jul 9 13:24:16 UTC 2019
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.
> 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
>>> 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 . 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
> 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.
More information about the core-libs-dev