[foreign] RFR: 8224244: Cleanup libclang Java API

[Maurizio Cimadamore]

I like both parts - nice job.

In JextractTool, what exactly is the point of catching the parse 
exception and rethrow? Wouldn't that be propagated out anyway?

Maurizio

On 22/05/2019 05:21, Henry Jen wrote:
> Updated webrev[1] change ParsingFailingException to extend RuntimeException and revert Handlers changes for declaring exception.
>
> I also have an add-on webrev[2] that instead of hard-code all built-in type, utilize reparse to find built-in types on demand. I don’t think it’s necessary better, just throw it as an option.
>
> Let me know what you think.
>
> [1] http://cr.openjdk.java.net/~henryjen/panama/8224244/1/webrev/
> [2] http://cr.openjdk.java.net/~henryjen/panama/8224244/1.1/webrev/
>
> Cheers,
> Henry
>   
>
>> On May 21, 2019, at 8:59 AM, Henry Jen <henry.jen at oracle.com> wrote:
>>
>>
>>> On May 21, 2019, at 1:25 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>>
>>> Looks very good - couple of comments:
>>>
>>> * I think it would be handy to collect all the translation unit options into an enum - I'm referring to these:
>>>
>>>   46     private final static int CXTranslationUnit_DetailedPreprocessingRecord = 0x01;
>>>
>>> That would have been really helpful when writing the reparsing code, when often I had to browse the clang headers to look for these.
>>>
>> Note that we didn’t expose an API to take options, so this constants are private, therefore I chose to remove those we don’t use. I thought about adding an API to take options and add all those constants, but didn’t go for it as we don’t need it for now, and this JNI binding is simple wrapper simply for jextract. Once runtime suppose is available, we really want to just use extracted API, which is why we have FFI jextract case. :)
>>
>> Regarding enum, I won’t recommend in this case, as they are flags to be combined with OR operator.
>>
>>> * it seem to me that if the parser catches the index exception and then wraps  it as a runtime exception (IllegalStateException) then you can revert most of the changes to the other classes? Also, another thing to consider: maybe we wanna make the parsing exception an unchecked exception? After all, there's not much recovery that can be done on those, forcing all clients to catch it seems excessive?
>>>
>> I think you are right, it’s better ParsingFailingException extends RuntimeException. I was thinking that we should force consideration of the case, but I agree that there is not much to do other than gracefully exit.
>>
>> Cheers,
>> Henry
>>
>>> Maurizio
>>>
>>> On 20/05/2019 21:53, Henry Jen wrote:
>>>> Hi,
>>>>
>>>> Please review the webrev[1] for clang Java API cleanup that came up with earlier work on atomic type support.
>>>> Mainly to throw a checked exception on parsing error, and move translation unit APIs into TranslationUnit class.
>>>>
>>>> Cheers,
>>>> Henry
>>>>
>>>> [1]
>>>> http://cr.openjdk.java.net/~henryjen/panama/8224244/0/webrev/
>>>>
>>>> [2]
>>>> https://bugs.openjdk.java.net/browse/JDK-8224244