On Tue, 10 Mar 2026 16:34:18 GMT, Paul Hübner <[email protected]> wrote:
>> Frederic Parain has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix generation of short values
>
> test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueClassGenerator.java
> line 146:
>
>> 144: c =
>> (char)random.nextInt(Character.MAX_VALUE+1);
>> 145:
>> 146: } while (c ==
>> Character.MAX_VALUE || c == Character.MIN_VALUE);
>
> The existence loop is really confusing to me. Can we not constrain the
> `nextInt` to be within 0 to (MAX-MIN) and then offset it by MIN? (Or adjust
> the bounds by -1, +1, etc., but the point is then we'd always get the correct
> number.)
I prefer to follow the same pattern for each primitive type.
> test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/ValueClassGenerator.java
> line 151:
>
>> 149: false, new String[] {"(char)1",
>> "Character.MAX_VALUE", "Character.MIN_VALUE"});
>> 150: primitiveTypes.add(new PrimitiveDesc("char", false, charVals));
>> 151: String[] shortVals = getPregeneratedValues(() -> {
>
> Thinking about it, we could get less complexity if we converted
> `Supplier<String>` to a Function<Set<String>, String>` whereby the `Set`
> represents all of the already computed values. So then our lambda becomes
> something like:
>
> String v = null;
> do {
> v = "(short)" + (random.nextInt(0, (shortMax - shortMin)) + shortMin);
> } while (existing.contains(v))
>
>
> Using a `Set` would also simplify `getPregeneratedValues` greatly. We won't
> need to do the duplicate checking etc. ourselves. I would suggest a
> `TreeSet` for backing so we are deterministic. We can really easily turn the
> `Set` into a `String[]` at the end before we return.
I want to preserve the hard coded values with their original writing
('MAX_VALUE' and others) when generating the source file of the test classes.
Still doable with the approach you propose, but then each value would have to
be stored in both String and numerical format. I don't think this complexity is
justified for such a minor piece of code.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2918299498
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2202#discussion_r2918293151
