[foreign-jextract] [Rev 01] RFR: 8239493: Add classfile generation to jextract

[Maurizio Cimadamore]

On Tue, 7 Apr 2020 10:10:52 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Hi,
>> 
>> This patch adds class file generation to jextract using ASM, which in some places replaces the generation done by the
>> current source code generator. In particularly, it replaces the generation of all constant fields with static getters,
>> that use dynamic constants as backing storage rather than fields. This allows for more sharing between constants, and
>> also reduces the pressure on clinit code size a lot, enabling extraction of much bigger native header files. The new
>> scheme generates an extra constant helper class which holds all these constants, and the source generator generates the
>> high-level wrapper functions. The scheme can also be extended to generate multiple constant helper classes, should we
>> run out of constant pool indices (in rare cases).  This patch reworks the interface of JavaSourceBuilder, since extra
>> parameters are needed in order to do the new constant generation. For generating methods that need constants, it calls
>> into a new ConstantHelper class, which returns a DirectMethodHandleDesc describing the generated constant getter. A
>> call to this getter is then emitted.  This patch also merges the HandleSourceFactory and StaticWrapperSourceFactory
>> into OutputFactory, since only the latter was ever being used, but the split in code made it a lot harder to reason
>> about the correctness of for instance changing C identifiers into java-safe ones. Because of this, the diff for
>> OuputFactory looks a bit messy, so maybe it's easier to look at the resulting file as a whole.  The tests are changed
>> to look for the now generated static getters, instead of fields (`static final` fields don't work well with dynamic
>> constants, since they are eagerly initialized).  As a small bonus, this patch adds the missing ABI-agnostic
>> C_LONGDOUBLE constant (this was causing some test failures).  Thanks, Jorn
>
> Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix incorrect copyright header

Looks really really solid. I've added some comments, most of which are minor suggestion to make the code more readable.
Thanks.

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

> 55:  * Scan a header file and generate Java source items for entities defined in that header
> 56:  * file. Tree visitor visit methods return true/false depending on whether a
> 57:  * particular Tree is processed or skipped.

Since you are touching this (but not an issue in this patch), I'd recommend renaming the predicates "visitedVariable"
and friends - which are very distracting (they almost look like the visitor methods :-) ).

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

> 128:     private String getFunction(String javaName, FunctionDescriptor fDesc) {
> 129:         return getterCall(constantHelper.addFunctionDesc(javaName, fDesc));
> 130:     }

I like the "flowy" nature of the new source builder ��

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

> 58:  */
> 59: public class OutputFactory implements Declaration.Visitor<Void, Declaration> {
> 60:     private final Set<String> constants = new HashSet<>();

Merging HandleSourceFactory and StaticWrapperSourceFactory makes sense ��

src/jdk.incubator.jextract/share/classes/jdk/incubator/jextract/tool/resources/RuntimeHelper.java.template line 56:

> 55:
> 56:     public static final MemoryAddress makeCString(String value) {
> 57:         value += '\0';

Why this? Don't we have the new Cstring type?

src/jdk.incubator.jextract/share/classes/jdk/incubator/jextract/tool/ConstantHelper.java line 201:

> 200:             if (type == int.class || type == byte.class || type == short.class || type == char.class) {
> 201:                 return emitGetter(name, type, mv -> emitConInt(mv, ((Long) value).intValue()));
> 202:             } else if (type == float.class) {

I think having `emitIntConstantGetter`, `emitFloatConstantGetter` could be clearer. I noted that emitGetter is always
used either through the condy case (which is fine) or through one of those constants getters here - I think *hiding*
the lambda might make the code more readable here (and then you can keep existing general `emiteGetter` as an helper
that is not to be used directly).

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

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