[foreign] RFR 8221471: jextract source genarator generates non-compilable classes with naming clashes

[Maurizio Cimadamore]

Looks good

Maurizio

On 10/04/2019 04:01, Sundararajan Athijegannathan wrote:
> Thanks for catching these...
>
> Updated webrev: 
> https://cr.openjdk.java.net/~sundar/8221471/webrev.03/index.html
>
> Responses to your review comments/test observations below..
>
>
> On 09/04/19, 11:07 PM, Jorn Vernee wrote:
>> Windows has some failures, seemingly from to va_list patch.
>>
>> 1.) small typo in IncompleArrayTest (for the Windows-only case):
>>
>> - 
>> assertNotNull(loader.loadClass(headerInterfaceName("incompleteArray3i.h")));
>> + 
>> assertNotNull(loader.loadClass(headerInterfaceName("incompleteArray3.h")));
>
>
> The va_list patch does not seem to have touched this file.
>
> https://mail.openjdk.java.net/pipermail/panama-dev/2019-April/005125.html
>
> Damage has been done by even earlier patch!
>
> https://mail.openjdk.java.net/pipermail/panama-dev/2019-March/005076.html
>
> My mistake! I should have all "all platforms" tests. Thanks for 
> catching this.
>
>>
>> 2.) ValistUseTest itself fails. It looks like the builtin$.h header 
>> is missing from src/jdk.jextract/share/conf for some reason [1], I 
>> guess it got dropped from the push by accident? But, after I added 
>> that the test still failed.
>
> I missed pushing builtin$.h as part of the va_list patch :( Added it 
> in the updated webrev above.
>
>>
>> I manually ran jextract on the test header, which works. The 
>> generated method uses Pointer<Byte> for the va_list, so it looks like 
>> the backing impl is different on Windows, and there's no need for 
>> jextract to generate the __va_list_tag struct in 
>> clang_support.builtin$_h that the test is checking for, so the test 
>> fails.
>>
>> Could go with adding `@requires os.family != "windows"` to 
>> ValistUseTest for that.
>
> As suggested, I made this test platform specific - va_list handling is 
> indeed platform specific.
>
>>
>> After those 2 fixes the tests are all green on my machine :)
>>
> Thanks for testing on Windows! [and welcome back! :) ]
>
> My mach5 "all platforms testing" with updated patch is successful as 
> well.
>
> Thanks,
> -Sundar
>
>> Cheers,
>> Jorn
>>
>> [1] : 
>> http://hg.openjdk.java.net/panama/dev/file/d6f2bf38b95b/src/jdk.jextract/share
>>
>> Maurizio Cimadamore schreef op 2019-04-09 18:20:
>>> Looks good!
>>>
>>> A multi-platform test run is probably better here :-)
>>>
>>> Maurizio
>>>
>>> On 09/04/2019 16:55, Sundararajan Athijegannathan wrote:
>>>> Please review.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8221471
>>>> Webrev: 
>>>> https://cr.openjdk.java.net/~sundar/8221471/webrev.02/index.html
>>>>
>>>> Current naming scheme: Header interfaces are named as "foo". Static 
>>>> forwarder class is named as "foo_h"
>>>> New naming scheme: Header interfaces are named as "foo_h". Static 
>>>> forwarder class is named as "foo_lib"
>>>>
>>>> Thanks,
>>>> -Sundar