On Thu, 31 Jul 2025 13:04:09 GMT, Jaikiran Pai <j...@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
