RFR: 8234089: (zipfs) Remove classes JarFileSystemProvider and JarFileSystem

Langer, Christoph christoph.langer at sap.com
Tue Nov 19 22:37:29 UTC 2019


Hi Lance,

> If you look at MultiReleaseJarTest.java, it explicitly references JAR FS in a
> comment. Same for JFSTester.java in the @Summary.  These should
> definitely be updated.  There are comments
> in ModulesInCustomFileSystem.java as well.

Ok, agreed for MultiReleaseJarTest and JFSTester. I fixed the comments in there. In ModulesInCustomFileSystem I can't see comments that explicitly refer to JarFileSystem.

> I would suggesting moving the code added to the constructor for checking
> the releaseVersion/multi-release properties to a method and grouping it
> with the other methods added for supporting MR jars around line 1396.
> (keeps it easier to follow and maintain)
> 
> Done that.
> 
> Thank you.  I do think however we need to change the name of the method
> to perhaps “checkReleaseVersionProperty” as the current name
> “initializeMultiReleaseJar”  does not seem quite right to me.

Ok, I changed it to " initializeReleaseVersion". I didn't follow your suggestion because the method not only checks the release version property but also triggers some initialization if a version property is found and it's an mr-jar.

> I would also update the comment for the method to something like:
> 
> ----------------
> Checks to see if the Zip File System property  “releaseVersion” has been
> specified.  If it has not been specified then check for the existence of the
> “multi-release” property.   If set and the file represents a multi-release JAR,
> determine the runtime version to use.
> ----------------

I updated the comment, trying to incorporate your suggestion. How do you like it?

> Could you also add a comment above the isMultiReleaseJar method to for
> clarity going forward (I know there was not one there before)
> 
> Done, too.
> 
> Thank you.
> 
> I think we can tweak the comment slightly to something like
> 
> ———————
> Returns true if the Manifest main attribute “Multi-Release” is set to true;
> false otherwise
> --------------------

Done.

> I might change the name of the lookup field in the method makeParentDirs
> with the addition of the Function lookup on line 126.
> 
> I chose to change the name of the global field in line 126 to be named
> mrlookup. Also updated the comment.
> 
> I am not sure “mrlookup” is the best name as this field is used with or
> without a multi-release jar.  Perhaps “IndexNodeNameLookup” or
> “entryLookup"

Ok, entryLookup seems the best fit to me. Changed that.

http://cr.openjdk.java.net/~clanger/webrevs/8234089.3/

Thanks again for reviewing.

Cheers
Christoph



More information about the core-libs-dev mailing list