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

Langer, Christoph christoph.langer at sap.com
Tue May 14 12:38:17 UTC 2019


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

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.

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

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

> o line 1245, comment to Sync
> • can you fix the typo udpate to update?

already addressed in webrev v2

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

obsolete now

> 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

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

Thanks
Christoph



More information about the core-libs-dev mailing list