On Fri, 18 Jul 2025 16:01:32 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> David Beaumont has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains one commit:
>>
>> Resync after benchmark.
>
> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 28:
>
>> 26:
>> 27: import java.io.IOException;
>> 28: import java.io.UncheckedIOException;
>
> Unused, can be removed
Done, thank's for the spot (I miss auto-import arrange and auto-format on save).
> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 279:
>
>> 277: }
>> 278:
>> 279: /// Returns a node in the JRT filesystem namespace, or null if
>> no resource or
>
> Using the standard javadoc tags for @param, @return can give some consistency
> and readability even for internal methods.
Absolutely. This sort of thing is always a balance between clarity and
conciseness. But here I think you're right so I pulled the `name` into a
`@param`. I dislike doing it for the return types in most cases though, since
that encourages duplication with the method's first sentence.
> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 532:
>
>> 530: public abstract static class Node {
>> 531: private final String name;
>> 532: private final BasicFileAttributes fileAttrs;
>
> It seems a bit wasteful to cache the same attributes in every Node.
> But its no more expensive (size-wise) than a referernce to the ImageReader
> and the indirection.
Quite, if I had to add any more fields here I'd have suggested making `Node` an
inner class. This opens up better API options (get buffer directly from the
node without going back to the reader) but also means that any users who keep
nodes beyond the scope of the reader that spawned then end up in trouble (since
the reader can be explicitly closed). I erred on the side of "don't change it"
since I didn't need a new field here.
> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 611:
>
>> 609: *
>> 610: * <p>If this node is not a directory ({@code isDirectory() ==
>> false})
>> 611: * then this method will throw {@link IllegalStateException}.
>
> A mismatch in overriding could give inconsistent results (since the impl does
> not call `isDirectory()`.
True, though would you prefer to change the comment ("by default this method
throws... and is overridden by directory subclasses...") or the implementation
of things like `getChildNames()` so they call `isDirectory()` ?
Personally I dislike this "test and call" approach and would rather have
restructured the API to be more object-oriented, and have callers use a more
structured dispatch mechanism (but this would incur cost of lambdas etc.), but
that's a really disruptive change.
Alternatively a type token/enum of some sort could be used to define node type.
This is all internal/trusted API though, so I'm happy with trusting that things
"do what they say" (it's going to be really obvious if something claims to be a
directory and then throws when asks for its child names, and that's almost
exclusively why anyone calls isDirectory() to start with).
So in summary, apart from maybe tweaking the comment slightly, I think this is
fine as is.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2218984216
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2218982421
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2218978525
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2218973869
