Thank you Lance. I pushed it with your suggestion worked in. From: Lance Andersen <lance.ander...@oracle.com> Sent: Donnerstag, 16. Mai 2019 15:44 To: Langer, Christoph <christoph.lan...@sap.com> Cc: core-libs-dev <core-libs-dev@openjdk.java.net>; nio-dev <nio-...@openjdk.java.net>; Alan Bateman <alan.bate...@oracle.com> Subject: Re: RFR: 8222276: (zipfs) Refactoring and cleanups to prepare for JDK-8213031
Hi Christoph, Thank you for the update. Looks good. One minor suggestion below that you can take care of before you push without a new webrev. Best, Lance On May 15, 2019, at 9:25 AM, Langer, Christoph <christoph.lan...@sap.com<mailto:christoph.lan...@sap.com>> wrote: Hi Lance, thanks for the quick turnaround. I tried to address your findings. Please find the new webrev here: http://cr.openjdk.java.net/~clanger/webrevs/8222276.3/ o FindEND() • I know that endsub and comlen fields are not currently used by Zip FS but given they are valid fields in the header we should probably add a comment that they are not currently used which is why they are commented out? there is a comment in line 1850 where the members of class END are declared. Shouldn't that suffice? Personally, I would add a comment about the unused members in the method to make it easier…. Ok, I've added comments. +1 o line 2061 version(boolean) • Could you add a comment to describe the method as the changes could be confusing to someone looking at this code for the 1st time addressed in v2, please check Definitely better but is still a bit confusing as it says stored =10 and zip64=stored (and the value is 45 ;-) ), can we tweak it a bit more Check it now 😊 Looks good • lines 2362 and 2462 are commented out but were not previously. Can you clarify why? the variables "pos" and "locPos" aren't used thereafter, so no need to increment them at these places. Maybe I should delete the lines altogether? OK, looking at this again (with extra coffee ;-) ) it makes sense and I would delete the lines. They are a leftover cut & paste code I suspect Deleted. :-) Can we move makeParentDirs() closer to buildNodeTree(), perhaps the same for removeFromTree()? Done. +1 Can the comment on line 1850 be worded better? I know you are trying to say that the lines commented out are not used but previously the fields were all together so being a bit more specific would help those coming later on to look at this code. Same for line 2025. Also can you add back the comment that writeCEN writes these fields out with a 0 value as we lost that comment? I updated this, please check. Can we tweak slightly to: The fields that are commented out below …... line 1937: can you fix the typo “tailing" which should be “trailing” in the comment. Done. + Does it look good now? It runs successfully through our regression testing system. Thanks and best regards Christoph [cid:image001.gif@01D50C9F.854199E0]<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>
