Hi Christoph, 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.
As far as variable names, I think this is nice to clean up but less important as part of this update. The good news is it is not a big set of changes for the comments so I would address those at a minimum. Best Lance > On Nov 19, 2019, at 4:03 PM, Langer, Christoph <christoph.lan...@sap.com> > wrote: > > 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.ander...@oracle.com > <mailto:lance.ander...@oracle.com>> > Sent: Dienstag, 19. November 2019 19:57 > To: Langer, Christoph <christoph.lan...@sap.com > <mailto:christoph.lan...@sap.com>> > Cc: core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net>; > nio-dev <nio-...@openjdk.java.net <mailto:nio-...@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.ander...@oracle.com > <mailto:lance.ander...@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.lan...@sap.com > <mailto:christoph.lan...@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/ > <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.ander...@oracle.com <mailto:lance.ander...@oracle.com> > > <image001.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.ander...@oracle.com <mailto:lance.ander...@oracle.com> > > > > <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.ander...@oracle.com <mailto:lance.ander...@oracle.com>
