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

Tue Nov 19 21:03:21 UTC 2019

Hi Lance,

I’m actually not so sure about this. While my cleanup change would remove the implementation detail of zipfs to use a class named “JarFileSystem” for multi-release jar peculiarities, I think a user of a FileSystem object upon a jar file is not wrong if he names the variable like jarFileSystem or the like. So I don’t see that such cleanup is really worth the while. Would you be ok with that?

I’ll get back to you soon with an updated webrev regarding the other changes you suggested.

Best regards
Christoph

From: Lance Andersen <lance.andersen at oracle.com>
Sent: Dienstag, 19. November 2019 19:57
To: Langer, Christoph <christoph.langer at sap.com>
Cc: core-libs-dev at openjdk.java.net; nio-dev <nio-dev at openjdk.java.net>
Subject: Re: RFR: 8234089: (zipfs) Remove classes JarFileSystemProvider and JarFileSystem

Hi Christoph,

Something else that should probably be done as part of this proposed change is  to clean up tests such as NodeCostDumpUtil.java and ModulesInCustomFileSystem.java and a few others as they have methods/fields etc... that make reference to the  Jar File System.  While it does not impact the tests as they are currently written, I think it would make sense to do this for clarity with the change you are making.

Best
Lance
On Nov 18, 2019, at 12:53 PM, Lance Andersen <lance.andersen at oracle.com<mailto:lance.andersen at oracle.com>> wrote:

Hi Christoph,

Sorry for the late reply but I was out of the office Friday
On Nov 15, 2019, at 3:40 AM, Langer, Christoph <christoph.langer at sap.com<mailto:christoph.langer at sap.com>> wrote:

Hi Lance,

thanks for reviewing. I've addressed your points:

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.

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

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

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"

This is the new webrev: http://cr.openjdk.java.net/~clanger/webrevs/8234089.2/

Cheers
Christoph

Thank you again!

Best
Lance

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

[cid:image001.gif at 01D59F25.2712D690]<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>