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