On Wed, 23 Jul 2025 06:37:34 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Feedback changes.
>
> src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line
> 259:
>
>> 257: ImageReader reader = SystemImage.reader();
>> 258: try {
>> 259: return reader.findNode("/modules").getChildNames().map(mn
>> -> readModuleAttributes(reader, mn));
>
> Can you put the line over 3 lines so that it's easier to see the pipeline.
Done.
> src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line
> 265:
>
>> 263: }
>> 264:
>> 265: // Every module is required to have a valid module-info.class.
>
> The methods in this class use use /** ..*/ in the method descriptions so
> would prefer if the changes change that. Here the method should really say
> that it returns the module's module-info, returning a holder for its class
> file attributes.
That comment was JavaDoc, just an implementation note (since it's private and
no separate doc would be created for it). I fleshed it out a bit though.
> src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line
> 276:
>
>> 274: err = e;
>> 275: }
>> 276: throw new Error("Missing or invalid module-info.class for
>> module: " + moduleName, err);
>
> I assume this can move into the catch block.
No because the return is conditional above "node != null && node.isResource()".
> src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line
> 457:
>
>> 455: * a non-resource node, then {@code null} is returned.
>> 456: */
>> 457: private ImageReader.Node findResourceNode(ImageReader reader,
>> String name) throws IOException {
>
> I think the usage would be clear if this were renamed to findResource.
Done.
> src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line
> 470:
>
>> 468: public Optional<ByteBuffer> read(String name) throws
>> IOException {
>> 469: ImageReader reader = SystemImage.reader();
>> 470: return Optional.ofNullable(findResourceNode(reader,
>> name)).map(reader::getResourceBuffer);
>
> Style wise, can you put the .map(...) on the next line so this is easier to
> read.
Done. Is there any documented best practice for this sort of line splitting
(since there isn't an enforce formatter).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2231629846
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2231634425
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2231631736
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2231628586
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2231626965
