[foreign] RFR 8212560 : jextract should generate a static forwarder class

[Jorn Vernee]

Sundararajan Athijegannathan schreef op 2018-10-17 14:37:
> On option: do we need an explicit option? given that one file extra
> and that too only when -l is specified. (too many options already? ;)
> )

Maybe having an option is nice, but have it be turned on by default? 
That way if someone really wanted to not generate the forwarder they 
could, and for the general use case they don't have to bother with 
setting an option every time.

> On naming: we've derived automatic names elsewhere. I think we could
> revisit all naming options by another round - perhaps even java
> convention for header interface name.
> 
> On subclassing: Nice suggestion. I made it as AsmCodeFactoryExt class
> and extended it from AsmCodeFactory. visit methods now return Boolean
> to tell if the particular tree was handled or not. This is needed so
> that subclass can avoid the same tree if super class avoided it (for
> eg. function-like macros, repeated definitions..)

I'm wondering if this runs into trouble later on when adding more code 
generation extensions/options. Let's say you add `foo` and `bar` options 
later on. You'd have to make an extension subclass for every combination 
of options (StaticFooBar, StaticFoo, StaticBar, FooBar, Static, Foo, 
Bar).

Maybe you could have something like a `CodeFactory`, which consists of 
an `AsmCodeFactory` and a list of `CodeFactoryExt`, of which the static 
forwarder generator would be one. And have this kind of pattern:

```
@Override
public Void visitVar(VarTree varTree, JType jt) {
     if(asmCodeFactory.visitVar(varTree, jt)) {
         for(CodeFactoryExt cfe : extensions) {
             cfe.visitVar(varTree, jt);
         }
     }
     return null;
}
```

I think that also allows you to get rid of the if/else statements in the 
extension class.

It's not really needed right now since there is only 1 extension, but it 
might be nice idea for the future.

Jorn

> Updated: 
> http://cr.openjdk.java.net/~sundar/8212560/webrev.01/index.html
> 
> Thanks,
> -Sundar
> 
> On 17/10/18, 3:25 PM, Maurizio Cimadamore wrote:
>> I like it! Few questions/comments:
>> 
>> * should it be enabled/disabled with explicit option
>> * should the name of the statics class be customizable
>> * I like the code organization - have you thought of pushing it 
>> further and make StaticForwarderGenerator a _subclass_ of 
>> AsmCodeFactory - each visitor could maybe delegate to super.visitXyz() 
>> first and then do its own bit? Then when you setup the pipeline, 
>> depending on what info is available on the context (e.g. presence of 
>> -l) you can decide whether to use a 'bare' ASMCodeFactory or the 
>> 'embellished' one. That should remove all the 'if staticFwd != null 
>> ...' sections from AsmCodeFactory.
>> 
>> Maurizio
>> 
>> 
>> On 17/10/2018 06:35, Sundararajan Athijegannathan wrote:
>>> Please review.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8212560
>>> Webrev: 
>>> http://cr.openjdk.java.net/~sundar/8212560/webrev.00/index.html
>>> 
>>> Thanks,
>>> -Sundar