RFR: 7903727: Remove the reliance on String Templates feature [v5]

[Jorn Vernee]

On Tue, 4 Jun 2024 10:13:12 GMT, Nizar Benalla <nbenalla at openjdk.org> wrote:

>> This PR aims to replace the usage of string templates with `String::format`, since there will be no string template feature in JDK 23.
>> I tried to keep similar indentation and convert them in-place, to make reviewing the changes easier.
>
> Nizar Benalla has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove space, to not appear in the diff

src/main/java/org/openjdk/jextract/impl/ClassSourceBuilder.java line 134:

> 132:     }
> 133:     void appendLines(String s, String... args) {
> 134:         sb.appendLines(String.format(s, (Object []) args));

Why is this `Object[]` cast needed here?

src/main/java/org/openjdk/jextract/impl/FunctionalInterfaceBuilder.java line 105:

> 103:         String paramStr = methodType.parameterCount() != 0 ? String.format(",%s", paramExprs()) : "";
> 104:         appendIndentedLines("""
> 105:         private static final MethodHandle DOWN$MH = Linker.nativeLinker().downcallHandle($DESC);

The indentation was changed here it looks like
Suggestion:

            private static final MethodHandle DOWN$MH = Linker.nativeLinker().downcallHandle($DESC);

src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java line 319:

> 317: 
> 318:             static {
> 319:             """);

Please keep the indentation, here and in other places.

src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java line 447:

> 445:                 MemorySegment.copy(varValue, 0L, %s.SEGMENT, 0L, %s.LAYOUT.byteSize());
> 446:             }
> 447:             """, javaName, holderClass, holderClass);

I suggest using numbers here too, since the param is repeated

src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java line 467:

> 465:                     }
> 466:                 }
> 467:                 """, javaName, indexList.decl(), holderClass, holderClass, indexList.use());

Param is repeated here too, so could re-use `%3$s`

src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java line 491:

> 489:             MemorySegment.copy(varValue, 0L, {0}({2}), 0L, {3}.byteSize());
> 490:         '}'
> 491:         """, javaName, indexList.decl(), indexList.use(), layoutString(elemType)));

Is this the only place where MessageFormat is used? Why just here?

(also, indentation was dropped here)

src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java line 583:

> 581:                 runtimeHelperName(),
> 582:                 Utils.quote(Objects.toString(value)),
> 583:                 constantName);

Numbers would be nice here to avoid repetition of parameters.

src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java line 591:

> 589:                 constantName,
> 590:                 constantValue(javaType, value));
> 591:                  emitDocComment(declaration);

Suggestion:

            emitDocComment(declaration);

src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java line 599:

> 597:                 javaType.getSimpleName(),
> 598:                 constantName,
> 599:                 constantName);

Numbers here too I think.

src/main/java/org/openjdk/jextract/impl/StructBuilder.java line 230:

> 228:         String valueParam = safeParameterName("fieldValue");
> 229:         appendIndentedLines("""
> 230:            public static void %1$s(MemorySegment %2$s, MemorySegment %3$s) {

Suggestion:

            public static void %1$s(MemorySegment %2$s, MemorySegment %3$s) {

src/main/java/org/openjdk/jextract/impl/StructBuilder.java line 315:

> 313:                 return {0}.asSlice(layout().byteSize() * index);
> 314:             '}'
> 315:             """, arrayParam));

This doesn't look right. The javadoc should not be changed here.

src/main/java/org/openjdk/jextract/impl/StructBuilder.java line 332:

> 330: 
> 331:             /**
> 332:              * Allocate a segment of size '{'@code layout().byteSize()'}' using {0}

Same here, we need to `@code` tag still.

src/main/java/org/openjdk/jextract/impl/ToplevelBuilder.java line 106:

> 104:                 files.add(header.toFile(currentSuffix,
> 105:                         s -> s.replace("#{SUFFIX}", currentSuffix)
> 106:                                .replace("#{PREV_SUFFIX}", prevSuffix)));

Suggestion:

                              .replace("#{PREV_SUFFIX}", prevSuffix)));

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

PR Review Comment: https://git.openjdk.org/jextract/pull/244#discussion_r1625844539
PR Review Comment: https://git.openjdk.org/jextract/pull/244#discussion_r1625861003
PR Review Comment: https://git.openjdk.org/jextract/pull/244#discussion_r1625879043
PR Review Comment: https://git.openjdk.org/jextract/pull/244#discussion_r1625873827
PR Review Comment: https://git.openjdk.org/jextract/pull/244#discussion_r1625874986
PR Review Comment: https://git.openjdk.org/jextract/pull/244#discussion_r1625876072
PR Review Comment: https://git.openjdk.org/jextract/pull/244#discussion_r1625895046
PR Review Comment: https://git.openjdk.org/jextract/pull/244#discussion_r1625896344
PR Review Comment: https://git.openjdk.org/jextract/pull/244#discussion_r1625898155
PR Review Comment: https://git.openjdk.org/jextract/pull/244#discussion_r1625901000
PR Review Comment: https://git.openjdk.org/jextract/pull/244#discussion_r1625910948
PR Review Comment: https://git.openjdk.org/jextract/pull/244#discussion_r1625913102
PR Review Comment: https://git.openjdk.org/jextract/pull/244#discussion_r1625864211