RFR: 8222276: (zipfs) Refactoring and cleanups to prepare for JDK-8213031
christoph.langer at sap.com
Wed May 15 13:25:39 UTC 2019
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.
> 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
> • 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
> Can we move makeParentDirs() closer to buildNodeTree(), perhaps the
> same for removeFromTree()?
> 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
I updated this, please check.
> line 1937: can you fix the typo “tailing" which should be “trailing” in the
Does it look good now? It runs successfully through our regression testing system.
Thanks and best regards
More information about the core-libs-dev