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

Nizar Benalla nbenalla at openjdk.org
Mon May 20 11:08:37 UTC 2024


On Mon, 20 May 2024 09:50:20 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Nizar Benalla has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Fix some indentations
>>  - Tweak `alignIfNeeded`
>
> src/main/java/org/openjdk/jextract/impl/ClassSourceBuilder.java line 145:
> 
>> 143: 
>> 144:     final void emitDefaultConstructor() {
>> 145:         appendIndentedLines(String.format("""
> 
> Suggestion - tweak the `appendIndentedLines` to call `String::format` inside that method, so that all clients don't have to call `String::format` manually from outside.

Would this work? 

Before

 appendIndentedLines(String.format("""
                private static final MethodHandle %s = %s.sliceHandle(%s);
                """,
                arrayHandleName, fieldLayoutName, path));

After 

void appendIndentedLines(String s, String... args) {
        sb.appendIndentedLines(String.format(s,args));
    }

and the call would look like 

appendIndentedLines("private static final MethodHandle %s = %s.sliceHandle(%s);"
                                , arrayHandleName, fieldLayoutName, path));

probably would create a new method rather than remove the old one

> src/main/java/org/openjdk/jextract/impl/ClassSourceBuilder.java line 193:
> 
>> 191:             case Primitive p -> primitiveLayoutString(p, align);
>> 192:             case Declared d when Utils.isEnum(d) -> layoutString(((Constant)d.tree().members().get(0)).type(), align);
>> 193:             case Declared d when Utils.isStructOrUnion(d) -> alignIfNeeded(String.format("%s.layout()", JavaName.getFullNameOrThrow(d.tree())), ClangAlignOf.getOrThrow(d.tree()) / 8, align);
> 
> Same thing here - if all calls to `alignIfNeeded` end up with `String::format`, considering moving `String::format` there

Fixed in [991ad7c](https://github.com/openjdk/jextract/pull/244/commits/991ad7c1a6f2c13aa9c7860cf654432e8bc70bac)

> src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java line 573:
> 
>> 571:             emitDocComment(declaration);
>> 572:             appendLines(String.format("""
>> 573:         public static %s %s() {
> 
> Why the negative indentation here? (also in the code below)

Fixed in [6adcab3](https://github.com/openjdk/jextract/pull/244/commits/6adcab3e23db3e03ee08a05889e23de31d088d2d), I think I didn't see them in the diff.

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

PR Review Comment: https://git.openjdk.org/jextract/pull/244#discussion_r1606603835
PR Review Comment: https://git.openjdk.org/jextract/pull/244#discussion_r1606604750
PR Review Comment: https://git.openjdk.org/jextract/pull/244#discussion_r1606604443


More information about the jextract-dev mailing list