RFR: Filter unsupported constructs in a separate pass

[Jorn Vernee]

On Fri, 1 Dec 2023 19:32:46 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> This PR moves all the logic to filter unsupported constructs in a dedicated visitor pass, hence removing all the checks from `OutputFactory`. This makes `OutputFactory` a lot simpler, as now this class only has to check for the presence of the `Skip` attribute.
> 
> One map in output factory (to keep track of generated structs) has been removed; we can in fact just set the `Skip` attribute on a scoped declaration as soon as we process it, to achieve the same effect. This map was also used in `visitTypedef`, so that we could fetch the correct supertype for the typedef declaration. This logic has now been replaced by making the `JavaName` attribute more expressive, so that we can deal with qualified names.
> 
> The other map could not be removed, and has been moved instead inside `NameMangler`. The issue is that we need some way to link a typedef type to the name of the functional interface associated with the typedef. Unfortunately, despite many attempts, I realized that the typedef type does not share anything in common with the typedef declaration; as such, piggy backing on existing attributes is not feasible. So for now, I kept a map, to retain existing behavior. It is possible that the IR might have to be tweaked to accommodate this use case.
> 
> Since `NameMangler` now correctly set the functional interface name on all variables whose type is either a function pointer, or a function pointer typedef, the logic in `OutputFactory` can, again, be simplified: now `OutputFactory` only needs to fetch the name attribute set by the mangler (if any).

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

> 36: import org.openjdk.jextract.Declaration;
> 37: import org.openjdk.jextract.Type;
> 38: import org.openjdk.jextract.impl.DeclarationImpl.JavaFunctionalInterfaceName;

Looks spurious.

src/main/java/org/openjdk/jextract/impl/NameMangler.java line 228:

> 226:             JavaFunctionalInterfaceName.with(variable, fiName);
> 227:         } else if (variable.type() instanceof Delegated delegatedType) {
> 228:             String typedefName = functionTypeDefNames.get(delegatedType.type());

You said (offline) that the type of the variable decl is not the same `Type` instance as the typedef's. But I suppose the are structurally equal, so we can actually find the attached fi name?

src/main/java/org/openjdk/jextract/impl/OutputFactory.java line 176:

> 174:             Type.Function f = Utils.getAsFunctionPointer(param.type());
> 175:             if (f != null) {
> 176:                 if (! generateFunctionalInterface(JavaFunctionalInterfaceName.getOrThrow(param), f)) {

Pre-existing, but this looks potentially problematic: it seems like it is possible that we generate a FI class for one of the parameters, then find that a following parameter is Skipped, and drop the function. Leaving a dangling FI class for the other param (similar if the return type is skipped). These FI classes are unique to the particular function AFAICS.

See also my related comment in UnsupportedFilter. I think this part can be simplified more by putting the Skip on the function tree instead. Then we would never see an unsupported/Skipped type here.

src/main/java/org/openjdk/jextract/impl/UnsupportedFilter.java line 164:

> 162:             warn("skipping " + JavaName.getOrThrow(decl) + " because of unsupported type usage: " +
> 163:                     unsupportedType);
> 164:             Skip.with(func);

I suggest putting the `Skip` on the Function tree of which this function type is a parameter/return type. I think that way we can simplify OutputFactory, and don't have to account for unsupported function types in the middle of `visitFunction` (see the early `null` returns in that method).

I wonder if variables and typedefs could also benefit from a similar treatment.

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

> 177:     }
> 178: 
> 179:     static Type.Visitor<String, Void> unsupportedVisitor = new Type.Visitor<>() {

Please capitalize the name, and make `private static final`.

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

> 240:     static void warn(String msg) {
> 241:         System.err.println("WARNING: " + msg);
> 242:     }

Can be `private` as well I think?

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

PR Review Comment: https://git.openjdk.org/jextract/pull/151#discussion_r1412522704
PR Review Comment: https://git.openjdk.org/jextract/pull/151#discussion_r1412528908
PR Review Comment: https://git.openjdk.org/jextract/pull/151#discussion_r1412533872
PR Review Comment: https://git.openjdk.org/jextract/pull/151#discussion_r1412541605
PR Review Comment: https://git.openjdk.org/jextract/pull/151#discussion_r1412539091
PR Review Comment: https://git.openjdk.org/jextract/pull/151#discussion_r1412545635