On Wed, 24 Sep 2025 18:54:02 GMT, Roger Riggs <[email protected]> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Manually deleting ImageReaderFactory (it returned somehow)
>
> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 333:
>
>> 331: // it references have that package marked as preview
>> only.
>> 332: // Skipping these entries avoids empty package
>> subdirectories.
>> 333: if (previewMode || (flags & FLAGS_IS_PREVIEW_ONLY) ==
>> 0) {
>
> Add utility methods for FLAGS_IS_PREVIEW_ONLY == 0
Done (in next push).
> src/java.base/share/classes/jdk/internal/jimage/ModuleReference.java line 122:
>
>> 120: public boolean hasContent() {
>> 121: return ((flags & FLAGS_HAS_CONTENT) != 0);
>> 122: }
>
> isEmpty could avoid introducing "content" as a loosely defined term.
> It would be more similar to directories being empty or lists being empty.
I've bounced between this several times. The thing is that a non-empty package
directory will not have content if it only contains other directories, and
seeing logic talking about a package being "empty" when it has child
directories is weird/confusing. I've actually thought about this naming quite
hard and I think it's more misleading to use the term "empty". However, since
it's obviously still not clear enough maybe a different name altogether?
How about "hasResources()" or "hasResourceContent()" ?
> src/java.base/share/classes/jdk/internal/jimage/PreviewMode.java line 65:
>
>> 63: FOR_RUNTIME() {
>> 64: @Override
>> 65: boolean resolve() {
>
> Each of these overrides creates an extra subclass.
> The alternative is for the resolve method use a switch on the ordinal to
> return or compute the boolean.
Excellent point, I'll fix this in the next push.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1619#discussion_r2414886599
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1619#discussion_r2414884864
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1619#discussion_r2414885908
