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

[Sundararajan Athijegannathan]

Updated: http://cr.openjdk.java.net/~sundar/8212560/webrev.02/index.html

Added --static-forwarder boolean option (default true)

-Sundar

On 17/10/18, 9:55 PM, Sundararajan Athijegannathan wrote:
>
> 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