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