[foreign-jextract] RFR: 8253725: Jextract fails to extract big monolithic headers

[Jorn Vernee]

On Mon, 28 Sep 2020 13:59:31 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> This patch adds support for splitting the symbols in the main header class into multiple classes; the header class is
> then rewrited to extend all these additional classes, so that all symbols would be exposed to users in the classic way.
> I've toyed with some ways to split on a by-header basis, but found that in certain cases we would generate near-empty
> classes - and that would still not prevent issues from occurring in the corner case a single very big header file
> contained so many symbols.  I've reorganized the code a bit - I noted that we were generating several nested classes,
> but in some cases (structs) we were using a builder, in other cases (functional interfaces, typedef) we weren't. Now we
> use builders for classes, and I've added an intermediate nested builder abstraction which does the right thing for all
> nested cases.  I've also converted the source constant generator to be a JavaSourceBuilder too - since it too is a
> class, after all, with a classBegin/End and build methods.  Finally, I moved the string builder/indentation logic out
> to a different class (StringSourceBuilder); since some builders want to have their own string buffer, while others (the
> nested ones) don't.  The organization of the builder code is still far from optimal, but I think that, moving forward,
> this consolidation should make it easier to make changes.

Looks great overall! I left some comments inline.

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/OutputFactory.java line 308:

> 306:                 }
> 307:                 MethodType fitype = typeTranslator.getMethodType(f, false);
> 308:                 toplevelBuilder.addFunctionalInterface(name, fitype,

Should `header()` be used here instead of toplevelBuilder as well?

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/OutputFactory.java line 356:

> 354:                              */
> 355:                             if (structDefinitionSeen(s)) {
> 356:                                 toplevelBuilder.addTypeDef(tree.name(), structDefinitionName(s), tree.type());

Same here; toplevelBuilder -> header() ?

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/SourceConstantHelper.java line 234:

> 232:         builder.append(OutputFactory.C_LANG_CONSTANTS_HOLDER);
> 233:         builder.append(".*;\n\n");
> 234:     }

Isn't this the same as in JavaSourceBuilder? Is the override needed?

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/ToplevelBuilder.java line 75:

> 73:         FunctionalInterfaceBuilder builder = new FunctionalInterfaceBuilder(this, name, mtype, desc, type);
> 74:         builder.classBegin();
> 75:         builder.classEnd();

This looks a little suspicious at first glance. It seems like it would just be a class open and close like:
class MyClass {
}
But actually the classEnd() method is adding some other things as a side effect. In other words, it looks like this
code _should_ tell me what is being added by the builder, but actually part of it is hidden.

I think this should either be made explicit here, e.g. having
builder.classBegin();
builder.addStuff();
builder.classEnd();
Or, we say that the entire building process is the responsibility of the FunctionalInterfaceBuilder, and here we just
have:

builder.build(); // does all 3
WDYT?

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/ToplevelBuilder.java line 81:

> 79:         TypedefBuilder builder = new TypedefBuilder(this, name, superClass, type);
> 80:         builder.classBegin();
> 81:         builder.classEnd();

Same here in that case.

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/ToplevelBuilder.java line 103:

> 101:         } else {
> 102:             declCount++;
> 103:             return lastHeader().orElse(this);

Does this mean the first 1000 decls always go in the top level interface? That makes sense, since for small headers
this still means you only get the top-level interface.

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

Marked as reviewed by jvernee (Committer).

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