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.forums2...@gmail.com >> <mailto:jai.forums2...@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
