RFR: 8344942: Template-Based Testing Framework [v71]

[Christian Hagedorn]

On Mon, 2 Jun 2025 14:10:29 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> test/hotspot/jtreg/compiler/lib/template_framework/NameSet.java line 88:
>> 
>>> 86:         if (w <= 0) {
>>> 87:             return null;
>>> 88:         }
>> 
>> Shouldn't the weight always be positive?
>
> Yes. True. I sometimes just also cover negative values to be a bit more robust... but I can also change it if you prefer that.

I guess if it's never negative, you can still cover it but maybe throw an exception instead?

>> test/hotspot/jtreg/compiler/lib/template_framework/Renderer.java line 266:
>> 
>>> 264:             case StringToken(String s) -> {
>>> 265:                 renderStringWithDollarAndHashtagReplacements(s);
>>> 266:             }
>> 
>> Suggestion:
>> 
>>             case StringToken(String s) -> renderStringWithDollarAndHashtagReplacements(s);
>
> I think I prefer the uniformity of the brackets as I have it. Would that be ok for you too?

I don't have a strong preference, so I'm fine with it 👍

>> test/hotspot/jtreg/compiler/lib/template_framework/TemplateBinding.java line 43:
>> 
>>> 41:      * Creates a new {@link TemplateBinding} that has no Template bound to it yet.
>>> 42:      */
>>> 43:     public TemplateBinding() {}
>> 
>> Can also be removed since it's the default constructor that is automatically added for you.
>> Suggestion:
>
> If I do that, then `javadoc` complains:
> 
> test/hotspot/jtreg/compiler/lib/template_framework/TemplateBinding.java:37: warning: use of default constructor, which does not provide a comment
> public class TemplateBinding<T extends Template> {

Ah I see. Then you can leave it in.

>> test/hotspot/jtreg/compiler/lib/template_framework/Token.java line 74:
>> 
>>> 72:             case Float s   -> outputList.add(new StringToken(Renderer.format(s)));
>>> 73:             case Boolean s -> outputList.add(new StringToken(Renderer.format(s)));
>>> 74:             case List l    -> parseList(l, outputList);
>> 
>> Not sure if we should use a raw `List` here. Would `List<?>` work as well? Would then need to update `parseList(List<Object> inputList ...)` to `List<?>` as well.
>
> What exactly do you think is the problem here?

My IDE advises against matching on the raw type `List`. As an alternative you can match on `List<?>`.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121921123
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121924453
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121918498
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121917521