RFR: 8222276: (zipfs) Refactoring and cleanups to prepare for JDK-8213031

[Lance Andersen]

Hi Chistoph,

> On May 14, 2019, at 8:38 AM, Langer, Christoph <christoph.langer at sap.com> wrote:
> 
> Hi Lance,
> 
>> Thank you for taking this on.
> 
> Thank you for reviewing! Unfortunately you didn't look at my latest edition... (http://cr.openjdk.java.net/~clanger/webrevs/8222276.2/)

Sorry, must have clicked on the wrong webrev.
> 
> In v2 I took back the modification of the hierarchy of classes IndexNode and Entry so it should be a bit easier to review. But I touched a few other places, compared to v1.
> 
OK
> Your comments still hold in parts, so let me answer them one by one:
> 
>> • ZipFileSystem
>> o initCEN()
>> • I think I understand why you made some of the changes here, but
>> for  initializing for example cen, and end, starting on line 137, was there a
>> reason you are doing this outside of the method now?
> 
> this is obsolete now, not part of the change any longer
ok
> 
>> 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….
> 
>> o line 1245, comment to Sync
>> • can you fix the typo udpate to update?
> 
> already addressed in webrev v2

Thank you
> 
>> o line 1928 IndexNode(IndexNode other) , is there a reason you left this code?
> 
> obsolete now

+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
> 
>> • 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 comment?

line 1937: can you fix the typo “tailing" which should be “trailing” in the comment.

Best
Lance

> 
> Thanks
> 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.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20190514/f0b4b52a/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: oracle_sig_logo.gif
Type: image/gif
Size: 658 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20190514/f0b4b52a/oracle_sig_logo.gif>