RFR: 7903605: Simplify TypeMaker [v2]

[Maurizio Cimadamore]

On Mon, 11 Dec 2023 10:58:26 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Address review comments
>
> src/main/java/org/openjdk/jextract/impl/TreeMaker.java line 143:
> 
>> 141:             default -> null; // skip
>> 142:         };
>> 143:         if (decl != null && pos != Position.NO_POSITION) {
> 
> If position is NO_POSITION, we do not cache. But I wonder, if there's no position (which indicates some clang intrinsics like va_list), should we really even generate a declaration? Both this approach, and a fix which simply returns `null` when position is `NO_POSITION` seem to work fine with all our tests.

There is a difference in the two approaches suggested above. Consider this example:


#include <stdarg.h>

struct Foo {
    int x;
    va_list v;
};


If we simply skip all declarations with NO_POSITION, we get a weird layout for Foo:


private static final MemoryLayout $LAYOUT = MemoryLayout.structLayout(
        JAVA_INT.withName("x"),
        MemoryLayout.paddingLayout(4),
        MemoryLayout.paddingLayout(24)
    ).withName("Foo");


Whereas with this patch we get this:


private static final MemoryLayout $LAYOUT = MemoryLayout.structLayout(
        JAVA_INT.withName("x"),
        MemoryLayout.paddingLayout(4),
        MemoryLayout.sequenceLayout(1,         MemoryLayout.structLayout(
                JAVA_INT.withName("gp_offset"),
                JAVA_INT.withName("fp_offset"),
                foo_h.C_POINTER.withName("overflow_arg_area"),
                foo_h.C_POINTER.withName("reg_save_area")
            ).withName("__va_list_tag")    ).withName("v")
    ).withName("Foo");


Both layouts are "correct". The struct interface won't have any accessor for the `v` field no matter which approach is taken (which makes sense, as we have no real declaration for `va_list`). While the second layout is more precise, I'm starting to think that the first approach is more honest: if we don't really have a cursor position for a declaration, and we cannot deduplicate, we cannot guarantee that declaration attributes won't be "lost" (as they might be added to different copies of the same underlying intrinsic cursor). WDYT?

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

PR Review Comment: https://git.openjdk.org/jextract/pull/155#discussion_r1422315850