[code-reflection] RFR: Lower StringTemplateOp

[Paul Sandoz]

On Wed, 13 Mar 2024 14:07:14 GMT, Mourad Abbay <mabbay at openjdk.org> wrote:

> The lowering of the StringTemplateOp. We chose not to have a block for every expression body, rather blocks will be created when necessary.

src/java.base/share/classes/java/lang/reflect/code/op/ExtendedOps.java line 2809:

> 2807:                     if (op instanceof YieldOp yop) {
> 2808:                         expressions.add(block.context().getValue(yop.yieldValue()));
> 2809:                         builders[0] = block;

Making a note for future change. This code suggests that `transformBody` could return the last block builder, so you don't have to keep track of it yourself. Another solution is to create blocks for the start of each expression and replace the yield with a branch to the next expression.

src/java.base/share/classes/java/lang/reflect/code/op/ExtendedOps.java line 2821:

> 2819:             Block.Builder builder = builders[0];
> 2820: 
> 2821:             Op.Result fragmentsList = builder.op(invoke(

The last parameter type of the method is `Object[]` but you are not packaging up the expression values into an array, which will require boxing of primitive values. This likely just happily works out in the interpreter because how it uses method handles, but likely fails when generating bytecode.

I am unsure if we should support this varargs-like behaviour as part of the invoke operation (and boxing). I am leaning towards not supporting.

My recommendation is to package up the expression values. Here's some example code (used for creating code models that build models). You would need to add boxing support.


    Value buildList(JavaType elementType, List<Value> elements) {
        JavaType listType = type(J_U_LIST, elementType);
        if (elements.size() < 11) {
            MethodRef listOf = MethodRef.method(J_U_LIST, "of",
                    J_U_LIST, Collections.nCopies(elements.size(), J_L_OBJECT));
            return builder.op(invoke(listType, listOf, elements));
        } else {
            Value array = buildArray(elementType, elements);
            return builder.op(invoke(listType, LIST_OF_ARRAY, array));
        }
    }


    Value buildArray(JavaType elementType, List<Value> elements) {
        Value array = builder.op(newArray(elementType,
                builder.op(constant(INT, elements.size()))));
        for (int i = 0; i < elements.size(); i++) {
            builder.op(arrayStoreOp(array, elements.get(i),
                    builder.op(constant(INT, i))));
        }
        return array;
    }

src/java.base/share/classes/java/lang/reflect/code/type/JavaType.java line 103:

> 101:     JavaType J_L_STRING_TEMPLATE_PROCESSOR = new JavaTypeImpl("java.lang.StringTemplate$Processor");
> 102: 
> 103:     JavaType J_L_LIST = new JavaTypeImpl("java.util.List");

Suggestion:

    JavaType J_U_LIST = new JavaTypeImpl("java.util.List");

test/jdk/java/lang/reflect/code/TestStringTemplateOp.java line 22:

> 20: 
> 21:     @CodeReflection
> 22:     static String f(int x, int y) {

Add a test that includes a conditional expression in a fragment expression.

Would be also useful to add a tests
- where fragment expressions return values of all primitive types and a reference type; and
- where the number of expressions is greater than the `List.of` parameter threshold where varargs is used.

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

PR Review Comment: https://git.openjdk.org/babylon/pull/39#discussion_r1525173968
PR Review Comment: https://git.openjdk.org/babylon/pull/39#discussion_r1525185355
PR Review Comment: https://git.openjdk.org/babylon/pull/39#discussion_r1525189750
PR Review Comment: https://git.openjdk.org/babylon/pull/39#discussion_r1525188244