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

[Sundararajan Athijegannathan]

comments below..

On 17/10/18, 9:13 PM, Jorn Vernee wrote:
> 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.
>
okay.

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

Right. We don't want to go there yet - but yes, if we happened to add 
one more, we can revisit & refactor the code.

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