[foreign-jextract] [Rev 01] RFR: 8238316: jextract emits a C_BOOL when source says char

[Jorn Vernee]

On Fri, 21 Feb 2020 12:23:03 GMT, Athijegannathan Sundararajan <sundar at openjdk.org> wrote:

>> Using SystemABI.Type of MemoryLayouts to generate layout constants
> 
> The pull request has been updated with 1 additional commit.

src/jdk.incubator.jextract/share/classes/jdk/incubator/jextract/tool/JavaSourceBuilder.java line 33:

> 32: import jdk.incubator.foreign.MemoryLayouts;
> 33: import jdk.incubator.foreign.MemoryLayouts.SysV;
> 34: import jdk.incubator.foreign.MemorySegment;

This import seems to be unused.
Suggestion:

src/jdk.incubator.jextract/share/classes/jdk/incubator/jextract/tool/JavaSourceBuilder.java line 141:

> 140:                 }
> 141:             } else {
> 142:                 sb.append(switch (type) {

`long double` is a standard type, so it should be fine to support on other platforms as well. e.g. Windows: https://docs.microsoft.com/en-us/cpp/cpp/fundamental-types-cpp?view=vs-2019

Looks like the constant is just missing from MemoryLayouts.WinABI. Should be 8 bytes there.
public static final ValueLayout C_LONGDOUBLE = MemoryLayout.ofValueBits(64, ByteOrder.LITTLE_ENDIAN)
        .withAnnotation(AbstractLayout.NATIVE_TYPE, SystemABI.Type.LONG_DOUBLE);

Looks like on arm it is 16 bytes, so you can copy the impl from SysV.

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/LayoutUtils.java line 66:

> 65:             case Char_U:
> 66:                 return C_UCHAR;
> 67:             case SChar:

Could use

            case UChar, Char_U:
                return C_UCHAR;

Here and elsewhere in the same switch.

Or maybe it's nice to switch the whole switch to a switch-expression using `->`, since then you can remove all the `return`s?

test/jdk/tools/jextract/testStruct/LibStructTest.java line 90:

> 89:          }
> 90:          assertEquals(fieldCount, 10);
> 91:     }

You could shorten this a bit using `MemoryLayout::select`:

private static void checkFieldABIType(GroupLayout group, String fieldName, SystemABI.Type expected) {
    assertEquals(group.select(groupElement(fieldName)).abiType().orElseThrow(), expected);
}

@Test
public void testFieldTypes() {
    GroupLayout g = (GroupLayout)AllTypes$LAYOUT;
    checkFieldABIType(g, "sc",  Type.SIGNED_CHAR);
    checkFieldABIType(g, "uc",  Type.UNSIGNED_CHAR);
    checkFieldABIType(g, "s",   Type.SHORT);
    checkFieldABIType(g, "us",  Type.UNSIGNED_SHORT);
    checkFieldABIType(g, "i",   Type.INT);
    checkFieldABIType(g, "ui",  Type.UNSIGNED_INT);
    checkFieldABIType(g, "l",   Type.LONG);
    checkFieldABIType(g, "ul",  Type.UNSIGNED_LONG);
    checkFieldABIType(g, "ll",  Type.LONG_LONG);
    checkFieldABIType(g, "ull", Type.UNSIGNED_LONG_LONG);
}

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

PR: https://git.openjdk.java.net/panama-foreign/pull/25