RFR: 7903933: Move sharable items from different generations to a common file [v8]

[Jorn Vernee]

On Mon, 5 May 2025 12:47:43 GMT, Nizar Benalla <nbenalla at openjdk.org> wrote:

>> Please review this patch to move the `C_*` layouts and the static utility methods into separate classes: `LayoutUtils.java` and `FFMUtils.java`, respectively.
>> 
>> - The names could later be personalized through a JSON configuration.
>> - We can use static imports if the `-t` option is no used and the files are generated into the default package, in that case we use the classname to call the static methods or use the `C_*` constants.
>> 
>> Some tests had to be modified slightly, either by adding new static imports or replacing classnames.
>
> Nizar Benalla has updated the pull request incrementally with one additional commit since the last revision:
> 
>   update test

doc/GUIDE.md line 989:

> 987: | `--dump-includes <String>`                                       | dump included symbols into specified file (see below)                                                                                                                                                                                                                                 |
> 988: | `--include-[function,constant,struct,union,typedef,var]<String>` | Include a symbol of the given name and kind in the generated bindings. When one of these options is specified, any symbol that is not matched by any specified filters is omitted from the generated bindings.                                                                        |
> 989: | `--symbols-class-name <name>`                                          | override the name of the root header class                                                                                                                                                                                                                                            |

Suggestion:

| `--symbols-class-name <name>`                                    | override the name of the root header class                                                                                                                                                                                                                                            |

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

> 43:     private FunctionalInterfaceBuilder(SourceFileBuilder builder, String className, ClassSourceBuilder enclosing,
> 44:                                        String runtimeHelperName, Type.Function funcType, boolean isNested) {
> 45:         super(builder, isNested ? "public final static" : "public final", Kind.CLASS, className, null, enclosing, runtimeHelperName);

Changes in this file seem unrelated? Leftover from other work?

src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java line 357:

> 355: 
> 356:             static final Arena LIBRARY_ARENA = Arena.ofAuto();""");
> 357:     }

Suggestion:

    void emitLibaryArena(){
        appendIndentedLines("""
            static final Arena LIBRARY_ARENA = Arena.ofAuto();""");
    }

src/main/java/org/openjdk/jextract/impl/IncludeHelper.java line 91:

> 89:     public String getSharedSymbolsFile() {
> 90:         return sharedSymbolsFile;
> 91:     }

Uhm, I think this should just be in `Options`, not in `IncludeHelper`, since it's not related to the `--include` options.

src/main/java/org/openjdk/jextract/impl/ToplevelBuilder.java line 46:

> 44:     public static final String PREV_SUFFIX = "#{PREV_SUFFIX}";
> 45:     private static final String SUFFIX = "#{SUFFIX}";
> 46:     private static String SHARED;

I think this was supposed to be `final`?
Suggestion:

    private final String shared;

src/main/resources/org/openjdk/jextract/impl/resources/Messages.properties line 88:

> 86: -t, --target-package <package>     target package name for the generated classes. If this option\n\
> 87: \                                   is not specified, then unnamed package is used.             \n\
> 88: --symbols-class-name <name>            override the name of the root header class                   \n\

Suggestion:

--symbols-class-name <name>        override the name of the root header class                   \n\

test/testng/org/openjdk/jextract/test/toolprovider/TestClassGeneration.java line 195:

> 193:     public void testFunctionalInterface(String name, MethodType type) {
> 194:         Class<?> fpClass = loader.loadClass("com.acme." + name);
> 195:         checkPrivateConstructor(fpClass);

Same here, these changes seem unrelated.

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

PR Review Comment: https://git.openjdk.org/jextract/pull/278#discussion_r2075975278
PR Review Comment: https://git.openjdk.org/jextract/pull/278#discussion_r2075987523
PR Review Comment: https://git.openjdk.org/jextract/pull/278#discussion_r2075966750
PR Review Comment: https://git.openjdk.org/jextract/pull/278#discussion_r2075970980
PR Review Comment: https://git.openjdk.org/jextract/pull/278#discussion_r2075974800
PR Review Comment: https://git.openjdk.org/jextract/pull/278#discussion_r2075982835
PR Review Comment: https://git.openjdk.org/jextract/pull/278#discussion_r2075988326