[foreign] RFR 8221154: jextract should generate java source files

[Maurizio Cimadamore]

Looks good!

Maurizio

On 20/03/2019 16:11, Sundararajan Athijegannathan wrote:
> Updated: https://cr.openjdk.java.net/~sundar/8221154/webrev.02/
>
> Removed JtregJextract driver use in Srcgentest.java.
>
> -Sundar
>
> On 20/03/19, 8:29 PM, Sundararajan Athijegannathan wrote:
>> Updated: https://cr.openjdk.java.net/~sundar/8221154/webrev.01/
>>
>> Changes:
>>
>> * Refactored JavaSourceBuilder.addMethod as you suggested.
>>
>> * yes, getSourceSignature(true) should have been used for arguments 
>> in addMethod. Thanks for catching! Fixed it.
>>
>> * On AsmCodeFactory/Ext duplication: yes, will get rid of ASM based 
>> generation in a future patch
>>
>> * Added basic test to dump sources and compile the same using javac 
>> ToolProvider. That uncovered an issue with NativeStruct annotation :)
>>
>> I've not modified JType's import interaction yet. I'll address that 
>> in a future patch - perhaps when removing ASM dependency.
>>
>> Thanks,
>> -Sundar
>>
>> On 20/03/19, 4:39 PM, Maurizio Cimadamore wrote:
>>> Hit send too fast; what about smoke-testing the feature by comparing 
>>> the output against a golden source file?
>>>
>>> Maurizio
>>>
>>> On 20/03/2019 11:08, Maurizio Cimadamore wrote:
>>>> Hi,
>>>> looks a solid piece of work! Comments:
>>>>
>>>> 1) In JType there seems to be a dependency on what the 
>>>> sourceSignature method generates and the set of imports generated 
>>>> by the source factory. I wonder if we could make the dependency 
>>>> explicit by adding another virtual method to JType which lists the 
>>>> packages it depends on (so that the source factory can collect them 
>>>> all in a set, and add an import at the end).
>>>>
>>>> 2) In JavaSourceBuilder::addMethod (both variant) shouldn't we use 
>>>> getSourceSignature(TRUE) for the parameter types?
>>>>
>>>> 3) Seems like there's a lot of duplication between the two 
>>>> JavaSourceBuilder::addMethod. The main differences seem to be where 
>>>> the function name comes from and the fact that the tree-based 
>>>> version also has (optional) parameter names; maybe we can rebase 
>>>> both methods on top of a third version which is more low level?
>>>>
>>>> The rest looks good; there's obvious duplication with 
>>>> AsmCodeFactory/Ext, but I think that's mostly by design (e.g. you 
>>>> started from those builders and replaced ASM calls with 
>>>> SourceBuilder calls, which is a fine approach).
>>>>
>>>> Maurizio
>>>>
>>>> On 20/03/2019 10:29, Sundararajan Athijegannathan wrote:
>>>>> Please review.
>>>>>
>>>>> This is initial change for moving jextract towards source 
>>>>> generation. The option --src-dump-dir will most likely be 
>>>>> changed/removed later. It is a development/debug option for now. 
>>>>> Reasons for source instead of direct .class generation are 
>>>>> documented in the bug report.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8221154
>>>>> Webrev: https://cr.openjdk.java.net/~sundar/8221154/webrev.00/
>>>>>
>>>>> Thanks,
>>>>> -Sundar