RFR: 7903590: Refactor jextract source code generation

[Jorn Vernee]

On Mon, 27 Nov 2023 12:22:18 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> We currently use `ClassSourceBuilder` both for top level classes and nested classes. That means for instance that we need to check whether to generate package declarations and import statements, and for nested classes we also emit code into a separate intermediate buffer that is copied into the enclosing buffer when we end the nested class. This PR proposes splitting part of the logic in `ClassSourceBuilder` into a separate `SourceFileBuilder`, which handles package decl/imports, and holds the underling `StringBuilder` which allows a nested class to share the `StringBuilder` with its enclosing class. Each `SourceFileBuilder` produces exactly one `JavaFileObject`. `TopLevelBuilder` and `Constants` now aggregate over `SourceFileBuilder`s instead of `JavaSourceBuilder`s. This also allows reducing the number of classes that implement `JavaSourceBuilder` to just `TopLevelBuilder` and `StructBuilder`, and allows dropping several methods from the interface. Other builders now wrap `Sourc
 eFileBuilder`, and emit code into its buffer.
>> 
>> Jextract source code generation related code currently relies on method overriding to allow sub classes to inject behavior into the generation code in a super class. This, however, makes it hard to follow what method implementation is actually being called when reading code. And, any method call may become 'suspicious' because it might do something non-obvious. For example, `emitConstructor` is overridden to do nothing in several sub-classes. But, this is not obvious when seeing the `emitConstructor()` call in `ClassSourceBuilder::classBegin`.
>> 
>> Furthermore, we use method overriding for instance to specify the class modifiers for a `ClassSourceBuilder` subclass, which means that control flow will flow through several class levels when generating code. This, and other cases, could be replaced by a simple (`String`) value passed to `ClassSourceBuilder` constructor. Using a direct field value in the generation code makes it clear that there is nothing special going on (as the method call might imply).
>> 
>> This PR proposes to remove most uses of method overriding, to make it easier to follow control flow when reading the code. This has a few implications:
>> -  Direct field access is used where possible, and data is passed to *Builder constructors instead of through a virtual method call to a sub class implementation
>> - `emitConstructor` was not needed in all cases. So, I've moved it from `ClassSourceBuilder::classBegin` to call...
>
> src/main/java/org/openjdk/jextract/impl/TypedefBuilder.java line 41:
> 
>> 39:     }
>> 40: 
>> 41:     void generate() {
> 
> This method seems only to be called after we create a new `TypedefBuilder`. Same is true for `FunctionalInterfaceBuilder`. It's also not a special method, just something that clients need to remember to call.
> I'm wondering if this patch isn't really trying to transition away from the "building" metaphore - e.g. what you want is to create a new "typedef", "functional interface" object, and add it to some list of generated objects (so that their code will be all co-generated at the end). Having the client to call `generate` explicitly so that the string buffer is filled seems redundant. Either the string buffer is filled at construction (eagerly), or it is filled when creating the java file object (lazily).

I don't see much point in doing this lazily, since we will always end up generating the code any way. What we could do is replace the `generate` method with a static one, that then also creates the builder internally.

Note that `FunctionalInterfaceBuilder` is called from both TopLevelBuilder and StructBuilder, so we can't just get rid of `FunctionalInterfaceBuilder` I think. We could potentially remove `TypedefBuilder` since it's only used int he former case, but I thought we should stick to the same pattern.

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

PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1406092109