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

[Jorn Vernee]

All good!

Jorn

Sundararajan Athijegannathan schreef op 2019-04-10 14:11:
> Updated again for the Linux test failure (private email):
> https://cr.openjdk.java.net/~sundar/8221471/webrev.04/
> 
> Only incremental change is in
> 
> https://cr.openjdk.java.net/~sundar/8221471/webrev.04/test/jdk/com/sun/tools/jextract/test8221838/StructTypedefsTest.java.sdiff.html
> 
> Library name changed to "Structtypedefs"
> 
> Please review.
> 
> -Sundar
> 
> On 10/04/19, 3:00 PM, Maurizio Cimadamore wrote:
>> 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