On Fri, 6 Mar 2026 12:45:05 GMT, Paul Hübner <[email protected]> wrote:
>> Frederic Parain has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Refactor generation of primitives pre-generated values
>
> test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/TestAbstractValueTemplate.java.template
> line 27:
>
>> 25: import java.lang.reflect.Field;
>> 26:
>> 27: <class_modifiers> <class_name> <super_class> {
>
> If this class is abstract, as this file name implies, it would be good to
> hardcore `abstract` here. I see that for both this and the value template,
> `<class modifiers>` is replaced with a constant string. Can we not have that
> in-line? It would make the test easier to read/understand w/o having to go
> back and forth.
I thought about that too, but putting the `abstract` key word here would then
split the class modifiers between the generator and the template. The currents
don't use additional modifiers yet, but I'd like to keep it possible. Same
reasoning applies to the non-abstract value class template, where all class
modifiers are handled by the generator.
> test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/TestAbstractValueTemplate.java.template
> line 29:
>
>> 27: <class_modifiers> <class_name> <super_class> {
>> 28: static Object[] predefined;
>> 29: <fields declarations>
>
> Nit: not following `snake_case` like other placeholders.
Fixed
> test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/TestValueTemplate.java.template
> line 92:
>
>> 90: }
>> 91:
>> 92: public boolean existInPredefinedValueSet() {
>
> Unused?
Removed
> test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueClassGenerator.java
> line 60:
>
>> 58: String path = System.getProperty("test.src");
>> 59: classTemplate = Files.readString(Path.of(path +
>> File.separator + "TestValueTemplate.java.template"));
>> 60: abstractValueClassTemplate = Files.readString(Path.of(path +
>> File.separator + "TestAbstractValueTemplate.java.template"));
>
> FYI: `File.separator` is not necessary, you can do
> `Files.readString(Path.of(path, "xyz.template"));`
Fixed
> test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueClassGenerator.java
> line 627:
>
>> 625: }
>> 626:
>> 627: void writeValueClasses() {
>
> I think this is the wrong approach. Instead, I would suggest to use
> `Path.createTempFile` which uses the `scratch` directory by jtreg and is the
> recommended way to create temporary files. We should not manually be cleaning
> the filesystem, the runner generally knows best.
Added creation of a new subdirectory in the `scratch` directory for each new
set of generated classes.
> test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueClassGenerator.java
> line 670:
>
>> 668: }
>> 669: ArrayList<String> optionList = new ArrayList<>();
>> 670: optionList.addAll(Arrays.asList("-source", "27"));
>
> Is there a programmatic way to get the current version? Hardcoding it will
> give us a headache if/when the version changes.
The programmatic way seems to be `Runtime.version().feature()`. Fixed.
> test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueRandomLayoutTest.java
> line 25:
>
>> 23:
>> 24: /*
>> 25: * @test
>
> Might need a randomness tag.
Added
> test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueRandomLayoutTest.java
> line 59:
>
>> 57: boolean useNullableNonAtomicFlat, String...
>> args) throws Exception {
>> 58: List<String> argsList = new ArrayList<>();
>> 59: Collections.addAll(argsList, "--enable-preview");
>
> Not necessary, `ProcessTools` will add it.
I've tried to remove it, but then the test fails because of an unsupported
class file version.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2907429611
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2907430151
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2907431114
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2907431938
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2907445222
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2907441503
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2907434502
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2907437171
