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