RFR: 8368333: [lworld] Add preview mode to ImageReader and JRT file-system [v3]
David Beaumont
duke at openjdk.org
Wed Oct 8 12:44:01 UTC 2025
On Wed, 24 Sep 2025 21:14:34 GMT, Roger Riggs <rriggs at openjdk.org> 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/jrtfs/JrtFileSystemProvider.java line 125:
>
>> 123: } else if (envValue instanceof String && Boolean.parseBoolean((String) envValue)) {
>> 124: return PreviewMode.ENABLED;
>> 125: }
>
> This kind of ambiguous type for the environment variable should not be supported.
> Pick a type and stick to it.
> Whereever the jrt: file system is specified will need a description for enablePreview to be added to the spec.
Other file-system flags already do this, so I was just copying that approach. Happy to not do this though.
Which do you suggest, Boolean or String?
> src/java.base/share/classes/jdk/internal/jrtfs/SystemImage.java line 79:
>
>> 77:
>> 78: // TODO: Maybe throw if enablePreview attempted for exploded image?
>> 79:
>
> Exploded image is useful for debugging, it could support preview mode using the same META-INF directory structure.
Indeed. I can support it later, I'm just wondering if, in this partially complete state, I should complain or not.
I've got most of the code for that done somewhere or other.
> test/jdk/jdk/internal/jimage/ModuleReferenceTest.java line 228:
>
>> 226:
>> 227: // Encodes strings sequentially starting from index 100.
>> 228: private static Function<String, Integer> testEncoder() {
>
> The method name `testEncoder` gives the impression of being a test, but it is just a utility function used by a test.
Goood point. It took me years to stop naming all my unit test methods testXxxx() so I should have spotted this. Thanks.
-------------
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2413721287
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2413724197
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2413726546
More information about the valhalla-dev
mailing list