On Thu, 12 Mar 2026 06:02:20 GMT, Jaikiran Pai <[email protected]> wrote:
>> David Beaumont has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Updated copyright
>> - Feedback changes
>
> src/java.base/share/classes/jdk/internal/jimage/BasicImageReader.java line
> 329:
>
>> 327: * files, and cannot be used to reliably lookup resources used at
>> runtime.
>> 328: *
>> 329: * <p>This API remains valid until the image reader from which it
>> was
>
> Nit - would the following be better:
>
>> <p>The returned {@code ResourceEntries} remains valid until the image reader
>> from which it was ...
Done.
> src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java line 121:
>
>> 119: * resource or directory:<br>
>> 120: * {@code FLAGS_IS_PREVIEW_VERSION}, and additionally {@code
>> 121: * FLAGS_IS_PREVIEW_ONLY} if no normal version of the resource
>> exists.
>
> Would it be more accurate to say "if no normal version of this resource or
> directory exists"?
Yes, good point.
> src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java line 132:
>
>> 130: * @return flags for the ATTRIBUTE_PREVIEW_FLAGS attribute.
>> 131: */
>> 132: public static int getFlags(String name, Predicate<String> hasEntry)
>> {
>
> Should this method be named `getPreviewFlags(...)`?
It could be, and was considered at the time. I don't recall why we didn't
though.
I think originally the attribute was just ATTRIBUTE_FLAGS though, so there was
a consistency argument that no longer applies.
I'll change it.
> src/java.base/share/classes/jdk/internal/jimage/PreviewMode.java line 80:
>
>> 78: InvocationTargetException e) {
>> 79: // But if the class exists, the method must exist
>> and be callable.
>> 80: throw new ExceptionInInitializerError(e);
>
> Hello David, `ExceptionInInitializerError` specifies that:
>
>> Signals that an unexpected exception has occurred in a static initializer.
>> An ExceptionInInitializerError is thrown to indicate that an exception
>> occurred during evaluation of a static initializer or the initializer for a
>> static variable.
>
> Throwing `ExceptionInInitializerError` doesn't seem appropriate. Perhaps
> throw a `IllegalStateException` with the `e` as the cause?
Good point, though I think AssertionError is appropriate here.
If I could just call the method directly, this would have been a compilation
failure (class exists, method was renamed/removed).
> src/java.base/share/classes/jdk/internal/jrtfs/SystemImage.java line 91:
>
>> 89: public static SystemImage fromDirectory(Path modulesDir, PreviewMode
>> mode) throws IOException {
>> 90: if (!Files.isDirectory(modulesDir)) {
>> 91: throw new FileSystemNotFoundException(modulesDir.toString());
>
> A `FileSystemNotFoundException` is not an `IOException`, should it instead
> just throw an `IOException`?
This is just me splitting existing code apart (see line 77 in the original).
I'd rather not change semantics/expectations in passing.
> src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java
> line 129:
>
>> 127: // Perform percent decoding of the resource name/path from the URL.
>> 128: private static String percentDecode(String path) throws
>> MalformedURLException {
>> 129: if (path.indexOf('%') == -1) {
>
> `ParseUtil.decode(...)` which is what gets called immediately in the next
> line already has this check, at entry, in its implementation. So I think this
> additional check at call sites, like here, isn't necessary.
It was deliberately pulled out here because of JDK-8359949 for which I want to
add proper decoding at some point (that's what the comment on line 133 is for).
I had hoped/expected that bug would have been fixed long before this migration
of the code.
So I'd rather keep it here so additional work at line 133 will be guarded
properly.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r2923890828
PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r2923950588
PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r2923918325
PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r2923805544
PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r2923996477
PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r2924031032
