RFR: 7903590: Refactor jextract source code generation
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 `SourceFi leBuilder`, 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::begin`. 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 instead. This allows the call to be omitted in some cases, without the need to define a sub class that overrides `emitConstructor` to do nothing. (Also renamed to `emitPrivateDefaultConstructor` for clarity). - Doc comments for methods need to be indented. This was previously handled by overriding the `emitDocComment` method in `HeaderFileBuilder` to add the indentation. Instead, I've moved the call to `emitDocComment` to the code generation `emit*` methods, which now handle the indentation for both the doc comment and the code together. - The `TopLevelBuilder.SplitHeader` and `FirstHeader` sub classes are no longer needed. They can be replaced by `HeaderFileBuilder`. I've create a factory method that takes over part of the code gen logic that `FirstHeader` was injecting by overriding `classBegin`. - Sub classes that need to inject extra code gen, now have their own generation methods which compose calls to `classBegin`/`classEnd` with other code gen methods. `classBegin` and `classEnd` now do the dumb/obvious thing. A good example of this is the new `generate` method in `FunctionalInterfaceBuilder`: void generate() { emitDocComment(funcType, className()); classBegin(); emitFunctionalInterfaceMethod(); emitFunctionalFactories(); emitFunctionalFactoryForPointer(); classEnd(); } I quite like how all the code here is in one place. ------------- Commit messages: - don't emit constructor for first header + inline FIB modifiers - remove extra changes - add newSourceFile factory method - use isNested() predicate - fix header comment indentation - reduce inheritance - SourceFileBuilder Changes: https://git.openjdk.org/jextract/pull/141/files Webrev: https://webrevs.openjdk.org/?repo=jextract&pr=141&range=00 Issue: https://bugs.openjdk.org/browse/CODETOOLS-7903590 Stats: 667 lines in 10 files changed: 236 ins; 258 del; 173 mod Patch: https://git.openjdk.org/jextract/pull/141.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/141/head:pull/141 PR: https://git.openjdk.org/jextract/pull/141
On Wed, 22 Nov 2023 07:38:29 GMT, Jorn Vernee <jvernee@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::begin`.
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 inst...
src/main/java/org/openjdk/jextract/impl/Constants.java line 89:
87: if (currentBuilder.constantIndex > CONSTANTS_PER_CLASS) { 88: currentBuilder.classEnd(); 89: currentBuilder = startNewBuilder(packageName, constantBuilders.size());
`currentBuilder` is never `null`, so we don't need to check for that. src/main/java/org/openjdk/jextract/impl/Constants.java line 231:
229: return kind == ClassSourceBuilder.Kind.CLASS ? 230: "static final " : ""; 231: }
Note that `Builder`s always have the `CLASS` kind, so we can just use a `static final String` for the modifiers. src/main/java/org/openjdk/jextract/impl/Constants.java line 251:
249: return constantName; 250: } 251:
This method was unused, so I've removed it. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1401624670 PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1401625856 PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1401625134
On Wed, 22 Nov 2023 07:38:29 GMT, Jorn Vernee <jvernee@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::begin`.
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 inst...
src/main/java/org/openjdk/jextract/impl/OutputFactory.java line 182:
180: 181: boolean isStructKind = Utils.isStructOrUnion(d); 182: JavaSourceBuilder prevBuilder = null;
Note that `StructBuilder` no longer keeps a reference to the enclosing builder. So, it can not be returned from `classEnd`/`end`. Instead, we keep track of the enclosing builder here. This also helps prevent StructBuilder from being too context aware. src/main/java/org/openjdk/jextract/impl/SourceFileBuilder.java line 119:
117: public JavaFileObject toFile(Function<String, String> finisher) { 118: return Utils.fileFromString(packageName, className, finisher.apply(sb.toString())); 119: }
Used by `TopLevelBuilder::toFiles` to replace the placeholder `#{SUPER}` super class name for the first header. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1401629941 PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1401632115
On Wed, 22 Nov 2023 07:38:29 GMT, Jorn Vernee <jvernee@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 165:
163: void append(Object o) { 164: sb.append(o); 165: }
This overload, and the one accepting a `boolean` were unused, so I've dropped them. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1401686657
On Wed, 22 Nov 2023 07:38:29 GMT, Jorn Vernee <jvernee@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
On Wed, 22 Nov 2023 07:38:29 GMT, Jorn Vernee <jvernee@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/JavaSourceBuilder.java line 38:
36: import java.util.Optional; 37: 38: interface JavaSourceBuilder {
I suggest to rename that to something like OutputHelper, OutputBuilder or something like that - or even nest this interface inside OutputFactory, to make it clear that the two are related (e.g. OutputFactory.Builder). src/main/java/org/openjdk/jextract/impl/JavaSourceBuilder.java line 50:
48: } 49: 50: default void addConstant(Declaration.Constant constantTree, String javaName, Class<?> javaType) {
Of all these methods, only `addVar/addStruct/addFunctionalInterface` really use virtual dispatch. So perhaps, in a follow up change, this abstraction can be cleaned up some more. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1406058217 PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1406058953
On Mon, 27 Nov 2023 12:00:09 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains ten additional commits since the last revision:
- Merge branch 'panama' into Untangle - Review comments - don't emit constructor for first header + inline FIB modifiers - remove extra changes - add newSourceFile factory method - use isNested() predicate - fix header comment indentation - reduce inheritance - SourceFileBuilder
src/main/java/org/openjdk/jextract/impl/JavaSourceBuilder.java line 38:
36: import java.util.Optional; 37: 38: interface JavaSourceBuilder {
I suggest to rename that to something like OutputHelper, OutputBuilder or something like that - or even nest this interface inside OutputFactory, to make it clear that the two are related (e.g. OutputFactory.Builder).
Moved it to an inner `OuputFactory.Builder` interface ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1406120511
On Wed, 22 Nov 2023 07:38:29 GMT, Jorn Vernee <jvernee@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 57:
55: private final String superName; 56: 57: ClassSourceBuilder(SourceFileBuilder builder, String modifiers, Kind kind, String className, String superName, List<String> enclosingClassNames) {
If this models a class, and classes can be nested, it is not clear to me as to why we wouldn't have a pointer to the enclosing class, rather than having just a list of names? src/main/java/org/openjdk/jextract/impl/SourceFileBuilder.java line 31:
29: import java.util.function.Function; 30: 31: public class SourceFileBuilder {
Should this class be final? ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1406062404 PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1406063946
On Mon, 27 Nov 2023 12:04:05 GMT, Maurizio Cimadamore <mcimadamore@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/ClassSourceBuilder.java line 57:
55: private final String superName; 56: 57: ClassSourceBuilder(SourceFileBuilder builder, String modifiers, Kind kind, String className, String superName, List<String> enclosingClassNames) {
If this models a class, and classes can be nested, it is not clear to me as to why we wouldn't have a pointer to the enclosing class, rather than having just a list of names?
Yeah, that might be simpler. I started out trying to remove the reference to enclosing classes altogether, but that turned out to be impossible since we might need to reference a nested class from source code we generate. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1406084491
On Mon, 27 Nov 2023 12:05:44 GMT, Maurizio Cimadamore <mcimadamore@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/SourceFileBuilder.java line 31:
29: import java.util.function.Function; 30: 31: public class SourceFileBuilder {
Should this class be final?
Yes. Also, it doesn't need to be `public`. The same goes for `FunctionalInterfaceBuilder` and `TypedefBuilder` ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1406103642
On Wed, 22 Nov 2023 07:38:29 GMT, Jorn Vernee <jvernee@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/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). ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1406083558
On Mon, 27 Nov 2023 12:22:18 GMT, Maurizio Cimadamore <mcimadamore@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
On Mon, 27 Nov 2023 12:30:19 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
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.
Not suggesting to remove those builders. But I think the client saying `new FunctionalInterfaceBuilder(...)` should be enough. E.g. my comment is really about reducing the "contract" between builders and their clients. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1406099372
On Mon, 27 Nov 2023 12:36:52 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
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.
Not suggesting to remove those builders. But I think the client saying `new FunctionalInterfaceBuilder(...)` should be enough. E.g. my comment is really about reducing the "contract" between builders and their clients.
Okay. In that case I'd like to go with the static `generate` method. In terms of contract surface that is the same, but it allows giving a name to the code generation 'side effect' as well. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1406110902
On Wed, 22 Nov 2023 07:38:29 GMT, Jorn Vernee <jvernee@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...
The changes look good. While in some cases (structs) we need to keep the builders "open", since we don't know how much stuff we're going to need to add, it seems to me that in other cases (typedef/functional interface) there's really no building process to speak of: we build a token of data model, which is then serialized into some java file object. ------------- Marked as reviewed by mcimadamore (Committer). PR Review: https://git.openjdk.org/jextract/pull/141#pullrequestreview-1750320331
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...
Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains ten additional commits since the last revision: - Merge branch 'panama' into Untangle - Review comments - don't emit constructor for first header + inline FIB modifiers - remove extra changes - add newSourceFile factory method - use isNested() predicate - fix header comment indentation - reduce inheritance - SourceFileBuilder ------------- Changes: - all: https://git.openjdk.org/jextract/pull/141/files - new: https://git.openjdk.org/jextract/pull/141/files/245c25dd..df918c7d Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=141&range=01 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=141&range=00-01 Stats: 204 lines in 14 files changed: 63 ins; 98 del; 43 mod Patch: https://git.openjdk.org/jextract/pull/141.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/141/head:pull/141 PR: https://git.openjdk.org/jextract/pull/141
On Mon, 27 Nov 2023 13:00:23 GMT, Jorn Vernee <jvernee@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...
Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains ten additional commits since the last revision:
- Merge branch 'panama' into Untangle - Review comments - don't emit constructor for first header + inline FIB modifiers - remove extra changes - add newSourceFile factory method - use isNested() predicate - fix header comment indentation - reduce inheritance - SourceFileBuilder
src/main/java/org/openjdk/jextract/impl/ClassSourceBuilder.java line 63:
61: this.className = className; 62: this.superName = superName; 63: this.enclosing = enclosing;
should we check that the enclosing class has the same source file builder as this one? ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1406156817
On Mon, 27 Nov 2023 13:26:21 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains ten additional commits since the last revision:
- Merge branch 'panama' into Untangle - Review comments - don't emit constructor for first header + inline FIB modifiers - remove extra changes - add newSourceFile factory method - use isNested() predicate - fix header comment indentation - reduce inheritance - SourceFileBuilder
src/main/java/org/openjdk/jextract/impl/ClassSourceBuilder.java line 63:
61: this.className = className; 62: this.superName = superName; 63: this.enclosing = enclosing;
should we check that the enclosing class has the same source file builder as this one?
Note really important from the `ClassSourceBuilder` perspective. Having builders with different underlying `SourceFileBuilders` would still work AFAICS, the sources would just be emitted to different source files. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/141#discussion_r1406199002
On Mon, 27 Nov 2023 13:00:23 GMT, Jorn Vernee <jvernee@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...
Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains ten additional commits since the last revision:
- Merge branch 'panama' into Untangle - Review comments - don't emit constructor for first header + inline FIB modifiers - remove extra changes - add newSourceFile factory method - use isNested() predicate - fix header comment indentation - reduce inheritance - SourceFileBuilder
Marked as reviewed by mcimadamore (Committer). ------------- PR Review: https://git.openjdk.org/jextract/pull/141#pullrequestreview-1750431301
On Wed, 22 Nov 2023 07:38:29 GMT, Jorn Vernee <jvernee@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...
This pull request has now been integrated. Changeset: 42076349 Author: Jorn Vernee <jvernee@openjdk.org> URL: https://git.openjdk.org/jextract/commit/420763490986d25c741a4368147ccea2f31c... Stats: 751 lines in 10 files changed: 255 ins; 327 del; 169 mod 7903590: Refactor jextract source code generation Reviewed-by: mcimadamore ------------- PR: https://git.openjdk.org/jextract/pull/141
participants (2)
-
Jorn Vernee
-
Maurizio Cimadamore