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

[Maurizio Cimadamore]

+1

Maurizio

On 10/04/2019 13:48, Jorn Vernee wrote:
> 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