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> > 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 > <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>
