RFR: 8360037: Refactor ImageReader in preparation for Valhalla support [v16]

[David Beaumont]

On Thu, 31 Jul 2025 13:04:09 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> David Beaumont has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Convert non-visible markdown comments to JavaDoc for consistency.
>
> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 189:
> 
>> 187:         private final Set<ImageReader> openers = new HashSet<>();
>> 188: 
>> 189:         // Attributes of the .jimage file. The jimage file does not contain
> 
> Nit - (pre-existing) calling it a `.jimage` file makes it look like `jimage` is a (well known) extension for jimage files. As far as I know, it isn't (the default jimage file that we ship in the JDK is just named `modules`). Perhaps we should change this to "Attributes of the jimage file."?

Good call. It's definitely not the file extension.

> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 195:
> 
>> 193: 
>> 194:         // Cache of all user visible nodes, guarded by synchronizing 'this' instance.
>> 195:         private final HashMap<String, Node> nodes;
> 
> Pre-existing, but since we are cleaning up some of this code, perhaps we can make this declaration a `Map` instead of a `HashMap`?

Done.

> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 316:
> 
>> 314:          * is not present in the cache.
>> 315:          */
>> 316:         Node buildModulesNode(String name) {
> 
> Should this be `private` method?

Done, ta.

> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 318:
> 
>> 316:         Node buildModulesNode(String name) {
>> 317:             assert name.startsWith(MODULES_ROOT + "/") : "Invalid module node name: " + name;
>> 318:             // Will fail for non-directory resources since the jimage name does not
> 
> Instead of "will fail for ...", should we reword it to "will return null for ..."

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2248042724
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2248040245
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2248045113
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2248047716