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

[Lance Andersen]

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> 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
> 

 <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>