RFR: 8222276: (zipfs) Refactoring and cleanups to prepare for JDK-8213031
Langer, Christoph
christoph.langer at sap.com
Wed May 15 13:25:39 UTC 2019
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.
> 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
Deleted.
> Can we move makeParentDirs() closer to buildNodeTree(), perhaps the
> same for removeFromTree()?
Done.
> 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.
> 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
More information about the core-libs-dev
mailing list