RFR: 7903605: Simplify TypeMaker [v2]

[Jorn Vernee]

On Mon, 11 Dec 2023 11:12:04 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> 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?

I think it may ultimately be preferable to keep as much of the layout intact, even if we don't generate accessors. That way, the user could do something with the layout themselves if they wanted to. I think maybe deduplicating based on `Position` only is not enough in the presence of intrinsics which do not have a position. But, we can have another look at better intrinsic support later as well. I think it's fine to just drop them for now.

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

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