[foreign-jextract] [Rev 03] RFR: JDK-8244270: reorganize ABI-dependent layout constants (second attempt)

[Jorn Vernee]

On Wed, 6 May 2020 11:28:04 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This is another attempt to reorganize ABI-dependent layout constants. The constant reorg is identical to the one
>> proposed in:
>>  https://git.openjdk.java.net/panama-foreign/pull/97
>> 
>> But this time I've also integrated the changes into the jextract branch (as illustrated in this PR).
>> 
>> The main realization which led to this work was that we don't need the ABI type enum: we already have the jextract
>> primitive type enum. So, we could just define constants for the common ABI types which are useful for ABI
>> classification in SystemABI - and then we could start from here in jextract, and augment these layouts by attaching an
>> extra attribute with jextract's Primitve/Kind enum.  I've simplified the API a bit by moving the layout inside the enum
>> itself (so that the layout can be accessed directly from the enum constant) - and also simplified the primitive type
>> factory.  Everything worked out as expected - note how much duplicated layout creation we managed to get rid of.
>> One problem I had was that the loader we use inside jextract test was not delegating correctly (was using `null` as
>> parent loader) so I had to fix that.
>> I'm not 100% sure that this jextract/type extra annotation makes sense - at the end of the day it is now only used
>> inside tests (as it makes layout comparison easier - since layouts might have additional attributes such as name and
>> such).  So I'm open to suggestion on whether we want to keep it or not - I believe that tests which look for specific
>> layouts should instead probably check for specific types instead. Anyway, we can also start from this cleanup, and take
>> care of these issues later; the important thing is that, with this rewrite we are finally able to cleanup the ABI
>> constants the way we wanted.
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update src/jdk.incubator.jextract/share/classes/jdk/incubator/jextract/Type.java
>   
>   Co-authored-by: Jorn Vernee <JornVernee at users.noreply.github.com>

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

> 198:             case 32: return Primitive.Kind.Int;
> 199:             case 64: return SystemABI.getSystemABI().name() == SystemABI.Win64.NAME ?
> 200:                     Primitive.Kind.LongLong : Primitive.Kind.Long;

Needs `equals()`
Suggestion:

            case 64: return SystemABI.getSystemABI().name().equals(SystemABI.Win64.NAME) ?

src/jdk.incubator.jextract/share/classes/jdk/incubator/jextract/tool/OutputFactory.java line 177:

> 176:             if (type.layout().isEmpty()) continue;
> 177:             String typeName = type.name().toLowerCase();
> 178:             MemoryLayout layout = type.layout().get();

Should use `typeName()` here, otherwise you get the `name()` implementation of the enum.
Suggestion:

            String typeName = type.typeName().toLowerCase().replace(' ', '_');
Either way, the name needs to match up with what is used by HeaderBuilder::emitPrimitiveTypedef

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

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