On Mon, 19 Feb 2024 10:41:03 GMT, Alan Bateman <al...@openjdk.org> wrote:

>> src/java.base/share/classes/sun/launcher/LauncherHelper.java line 821:
>> 
>>> 819:         // In LM_JAR mode, put the underlying file in the 
>>> JarFile/ZipFile cache.
>>> 820:         // This will avoid needing to re-parse the manifest when the 
>>> JAR file
>>> 821:         // is opened on the class path, triggered by Class.forName 
>>> below.
>> 
>> Nice improvement to have. would it be ok to pad out this comment a bit more 
>> ? I found it difficult to understand the specifics. Perhaps :
>> 
>> 
>>              // In LM_JAR mode, keep the underlying file open to retain it 
>> in 
>>              // the ZipFile HashMap cache. This will avoid needing to 
>> re-parse
>>                 // the central directory when the file is opened on the 
>> class path,
>>              // triggered by Class.forName below.
>
>> Nice improvement to have. would it be ok to pad out this comment a bit more 
>> ? I found it difficult to understand the specifics. Perhaps :
>> 
>> ```
>>              // In LM_JAR mode, keep the underlying file open to retain it 
>> in 
>>              // the ZipFile HashMap cache. This will avoid needing to 
>> re-parse
>>                 // the central directory when the file is opened on the 
>> class path,
>>              // triggered by Class.forName below.
>> ```
> 
> No issue with expanding the comment but I think leave it as "ZipFile cache" 
> as a more specific comment will quickly bit rot and confuse reader, and of 
> course the Launcher code doesn't care which collection is used internally in 
> the ZipFile code.

I'll expand the comment as suggest, keeping the "JarFile/ZipFile cache" phrase.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1494722308

Reply via email to