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

[Sundararajan Athijegannathan]

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