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

[Langer, Christoph]

Thank you Lance. I pushed it with your suggestion worked in.

From: Lance Andersen <lance.andersen at oracle.com>
Sent: Donnerstag, 16. Mai 2019 15:44
To: Langer, Christoph <christoph.langer at sap.com>
Cc: core-libs-dev <core-libs-dev at openjdk.java.net>; nio-dev <nio-dev at openjdk.java.net>; Alan Bateman <Alan.Bateman at oracle.com>
Subject: Re: RFR: 8222276: (zipfs) Refactoring and cleanups to prepare for JDK-8213031

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.langer at sap.com<mailto:christoph.langer at 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

[cid:image001.gif at 01D50C9F.854199E0]<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/20190517/c13771ac/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.gif
Type: image/gif
Size: 658 bytes
Desc: image001.gif
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20190517/c13771ac/image001-0001.gif>