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

[Sundararajan Athijegannathan]

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