RFR: 8234089: (zipfs) Remove classes JarFileSystemProvider and JarFileSystem
Lance Andersen
lance.andersen at oracle.com
Tue Nov 19 18:57:23 UTC 2019
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> 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/ <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.Andersen at oracle.com <mailto:Lance.Andersen at 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.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
More information about the core-libs-dev
mailing list