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

[Roger Riggs]

On Mon, 21 Jul 2025 11:57:38 GMT, David Beaumont <duke at openjdk.org> wrote:

>> 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.

The {@return 1st sentence text} can be used on the first line and javadoc will put the text in both places without needing duplication in the source.  [(javadoc  tag specification](https://docs.oracle.com/en/java/javase/22/docs/specs/javadoc/doc-comment-spec.html))

>> 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.

I would change the comment.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2222946949
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2222934325