RFR: 7903449: Jextract generates structs that cannot be compiled [v3]

[Jorn Vernee]

On Fri, 24 Mar 2023 18:22:46 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> While most of the constants required by jextract are stored in the auxiliary constant$xyz files, some constants are stored inside the class declaration corresponding to a struct. This was done to minimize the number of constant classes being generated. However, this can backfire, as when the generated struct is big, too many constants are added to it, which can result in a source code which fails to compile (as it's too big).
>> 
>> Instead of doing something even more convoluted, I've decided to centralize the generation of _all_ constants under the constant$xyz files. This means that there should be no constants generated in structs (or any other "public" declaration).
>> 
>> Doing this was problematic, as the current arrangement somehow limits the name clashes when adding constants with same name to different structs. After some attempts, I realized that the best way forward would be to simplify the `ConstantBuilder` class, so that it no longer cares about "public" Java names. Instead that class should only be used to generate declaration for constants that are used internally, by the jextract bindings. Because of that, _any_ name will do - for this reason this patch switches to a regular naming schemes for all constants, like `const$0`, `const$1`, etc.
>> 
>> Note that these are **not** the names exposed by jextract bindings - such names (the names of the constant *getters*) are still the same as before.
>> 
>> The main changes are around `ConstantBuilder`, which is now gone. In its place we find `Constants`, which acts as a sort of source-based constant pool. `Constants` has a cache (where already defined constants of certain kinds are stored), and new constant declarations are only emitted when needed. I've also simplified the API which is used by clients to generate constants, to remove the need of lambda expressions (which seemed a bit over the top).
>> 
>> I've also centralize the generation of numeric constants under `Constants` as well (before, these constants where generated on the fly in `HeaderFileBuilder`).
>> 
>> And, I also realized that our setup for sharing primitive layout constants was too complex. Most primitive layout constants are just wired to some constant in `ValueLayout`. The only special case is `C_POINTER`, which needs its *target layout* set (and presumably we only want to do that once). A much simpler solution is to just store `C_POINTER` inside `RuntimeHelper` and just use that.
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Share more constants

Very nice

src/main/java/org/openjdk/jextract/impl/Constants.java line 423:

> 421:         }
> 422: 
> 423:         private Constant emitConstantSegment(Object value) {

Maybe this should be renamed to `emitConstantString` (if you're here any way...)

src/main/java/org/openjdk/jextract/impl/Constants.java line 506:

> 504:         record DowncallKey(FunctionDescriptor desc) { }
> 505:         DowncallKey downcallKey = new DowncallKey(descriptor);
> 506:         Constant constant = cache.get(downcallKey);

Isn't dropping the names from the layouts required here as well?

src/main/java/org/openjdk/jextract/impl/FunctionalInterfaceBuilder.java line 190:

> 188:     private String prefixName() {
> 189:         return qualifiedName(this) + "$FI";
> 190:     }

These seem to be unused?

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

PR Review: https://git.openjdk.org/jextract/pull/117#pullrequestreview-1361422090
PR Review Comment: https://git.openjdk.org/jextract/pull/117#discussion_r1150842065
PR Review Comment: https://git.openjdk.org/jextract/pull/117#discussion_r1150840046
PR Review Comment: https://git.openjdk.org/jextract/pull/117#discussion_r1150845573