RFR: Filter unsupported constructs in a separate pass

[Maurizio Cimadamore]

On Fri, 1 Dec 2023 20:05:12 GMT, Jorn Vernee <jvernee 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/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?

Yes, they are structurally equal (the old code also relied on this property)

> 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.

Very good suggestion - thanks!

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

PR Review Comment: https://git.openjdk.org/jextract/pull/151#discussion_r1412605429
PR Review Comment: https://git.openjdk.org/jextract/pull/151#discussion_r1412613343