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