Hello Lance, On 10/07/19 2:25 AM, Lance Andersen wrote: > Hi Jaikiran, > > Again, thank you for your efforts here. > > Is there a CSR for this yet?
There isn't one yet. I will need help on this part, since I haven't created a CSR before. Is this something that I can create (I'm just a "author" in the JDK project)? If yes, is there some specific things I need to do? I'll reply to the rest of the review comments soon. Thank you very much for doing this review and offering to sponsor. -Jaikiran > This will need to approved prior to moving forward with committing the > feature. > > I can sponsor once everything has been approved and finalized. > > > > > ——— > @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. 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 > > - Undefined behavior after close()/end() > > I am not convinced the new wording is an improvement and I know Stuart > was not thrilled with the existing wording. However given the classes > may be subclassed, I am not sure we can truly specify the behavior > which could be why the original authors used that wording. Stuart > thoughts? > > > 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 > > > > >> On Jul 9, 2019, at 9:18 AM, Jaikiran Pai <jai.forums2...@gmail.com >> <mailto:jai.forums2...@gmail.com>> wrote: >> >> Can I please get a review and a sponsor for the patch that implements >> the enhancement noted in >> https://bugs.openjdk.java.net/browse/JDK-8225763 ? >> >> There has been a recent discussion about this change in the >> core-libs-dev mailing list here[1]. The latest version of the patch for >> this change is now available at [2]. >> >> Here's a summary of what's contained in this patch: >> >> - The Inflater/Deflater class now implements AutoCloseable >> >> - The class level javadoc of both these classes has been to updated to >> use newer style of code, with try-with-resources, for the example >> contained in that javadoc. The @apiNote in these class level javadoc has >> also been updated to mention the use of close() method for releasing >> resources. >> >> - 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 >> >> - The new close() method has been added along with javadoc which uses an >> @implSpec to state that it internally calls 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 >> >> - 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 > > > Again, thank you for your efforts here. > > 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> > > >
