On Mon, 21 Jul 2025 11:57:38 GMT, David Beaumont <d...@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
