On Sun, 25 Aug 2024 15:08:27 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Jaikiran Pai has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - revert to old code comment
>> - use "JAR file" consistently in test's code comments
>> - rename closeLoaderIgnoreEx to closeQuietly
>
> src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 447:
>
>> 445: loader = getLoader(url);
>> 446: // If the loader defines a local class path then
>> construct
>> 447: // and add the URLs as the next URLs to be opened.
>
> The change to URLClassLoad looks okay although I think the original comment
> was a bit clearer, no need to insert "then construct" here as this is just
> getting the class path URLs.
Hello Alan, I've reverted the change to this code comment in the latest update
to this PR.
> src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 453:
>
>> 451: if (DEBUG) {
>> 452: System.err.println("Failed to construct a loader or
>> construct its" +
>> 453: " local classpath for " + url + ", cause:"
>> + e);
>
> At some point we should probably get rid of the DEBUG stuff, it looks like it
> was left over from early JDK releases.
The debug logs are currently enabled through the `sun.misc.URLClassPath.debug`
system property. Do you mean we should completely get rid of the
`sun.misc.URLClassPath.debug` system property in future?
> src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 482:
>
>> 480:
>> 481: // closes the given loader and ignores any IOException that may
>> occur during close
>> 482: private static void closeLoaderIgnoreEx(final Loader loader) {
>
> closeQuietly(Loader loader) would work too.
I had considered `safeClose(...)` but that didn't sound right. But what you
suggest is more precise. I've updated the PR to use this suggestion.
> test/jdk/java/net/URLClassLoader/JarLoaderCloseTest.java line 78:
>
>> 76:
>> 77: /*
>> 78: * Creates a jar with a manifest which has a Class-Path entry value
>> with malformed URLs.
>
> I think you mean a "a JAR file" rather than "jar" here. Same nit in
> testParsableClassPathEntry.
Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1730676789
PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1730677519
PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1730677963
PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1730678041
