RFR: 7903590: Refactor jextract source code generation

[Jorn Vernee]

On Wed, 22 Nov 2023 07:38:29 GMT, Jorn Vernee <jvernee 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 `Source
 FileBuilder`, 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 callers of that method...

src/main/java/org/openjdk/jextract/impl/ClassSourceBuilder.java line 75:

> 73: 
> 74:     String className() {
> 75:         return desc.displayName();

Note that `desc.displayName()` is inaccurate for nested classes, as it will return e.g. `A$B` for a nested `A.B` class. This is then impossible to distinguish from class names that actually have a `$` in the name. I felt that it was better to just use a `String` to represent the simple class name instead.

src/main/java/org/openjdk/jextract/impl/ClassSourceBuilder.java line 179:

> 177:     }
> 178: 
> 179:     final void emitConstantGetter(String mods, String getterName, boolean nullCheck, String symbolName, Constant constant, Declaration decl) {

This method is only used by `Constants,Builder` so maybe it should be moved there. I've left it for now.

src/main/java/org/openjdk/jextract/impl/ClassSourceBuilder.java line 242:

> 240:         return className().equals(name) ||
> 241:                 (isNested() && enclosing.isEnclosedBySameName(name));
> 242:     }

Moved to `StructBuilder`, which is the only user of this method.

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

PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1401694780
PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1401697899
PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1401697003