[foreign] RFR 8218281: jextract native library related command line options should be revisited

[Maurizio Cimadamore]

Looks good

Thanks
Maurizio

On 05/02/2019 16:56, Sundararajan Athijegannathan wrote:
> Thanks for the review. Removed WARN mode and adjusted "action" as a 
> lambda in MissingSymbolAction enum as suggested.
>
> Updated: http://cr.openjdk.java.net/~sundar/8218281/webrev.02/
>
> Thanks,
> -Sundar
>
> On 05/02/19, 6:03 PM, Maurizio Cimadamore wrote:
>> Looks good.
>>
>> One minor question/comment is if you thought about replacing the 
>> MissingActionEnum with lambda expression set up in Main - e.g. 
>> MissingActionEnum could be, essentially a Consumer<String>.
>>
>> I'm saying this because most of the action refer to methods in Main 
>> itself (e.g. format), and the actions are also simple enough that I 
>> think the code would not be too bad?
>>
>> Or, you could merge the enum and lambda approach together:
>>
>>
>> public enum MissingSymbolAction {
>>      ERROR(name -> throw new 
>> RuntimeException(Main.format("err.symbol.not.found", name)),
>>      EXCLUDE(name -> err.println(Main.format("warn.symbol.excluded", 
>> name)),
>>      WARN(name -> err.println(Main.format("warn.symbol.not.found", 
>> name)),
>>      IGNORE(name -> {});
>>
>>      final Consumer<String> action;
>>
>>      ...
>> }
>>
>>
>> Then SymbiolFilter can just fetch the action corresponding to the 
>> enum constant, and execute it with given name, no need for a switch 
>> (or, maybe you do to be able to 'return null' in case symbol is 
>> excluded).
>>
>>
>> Also, looking back, I'm not 100% sure of the need for the distinction 
>> between WARN and EXCLUDE. They basically do the same thing - they 
>> produce a warning - but the latter also omits the symbol from the 
>> resulting API. The question is - is there ever a case where it makes 
>> sense to generate an API point under these circumstances? Wouldn't 
>> the code always fall over at runtime when invoking that method (or, 
>> more likely, early when binding the interface) ?
>>
>> So, to me ERROR is good - we want to fail if some mismatch is detected.
>>
>> If we don't want to fail, then I think we should take action towards 
>> generating something that "makes sense" - at which point the only 
>> distinction I see is: do I want to be bothered about (WARN), or not 
>> (IGNORE) ?
>>
>>
>> And, last (general) comment, we need to capture these options 
>> somewhere in the MD file.
>>
>> Maurizio
>>
>>
>> On 04/02/2019 16:22, Sundararajan Athijegannathan wrote:
>>
>>> Hi Anthony,
>>>
>>>
>>> Thanks for spotting those!
>>>
>>>
>>> Updated: https://cr.openjdk.java.net/~sundar/8218281/webrev.01/
>>>
>>>
>>> Thanks,
>>> -Sundar
>>>
>>>
>>> On 04/02/19, 9:25 PM, Anthony Vanelverdinghe wrote:
>>>>
>>>> Hi Sundar
>>>>
>>>> In JModWriter.java there is:
>>>>
>>>> -            if (ctx.libraryPaths.isEmpty() && 
>>>> ctx.linkCheckPaths.isEmpty()) {
>>>>
>>>> +            if (ctx.libraryPaths.isEmpty() && 
>>>> ctx.libraryPaths.isEmpty()) {
>>>>
>>>> but now both conditions are the same, so the second can be removed.
>>>>
>>>> In Main.java there is an unintended whitespace change:
>>>>
>>>> -        ctx.setGenStaticForwarder(staticForwarder && 
>>>> options.has("l"));
>>>>
>>>> +        ctx.setGenStaticForwarder(staticForwarder&& 
>>>> options.has("l"));
>>>>
>>>> Kind regards
>>>>
>>>> Anthony
>>>>
>>>> ------------------------------------------------------------------------ 
>>>>
>>>> *From:* panama-dev <panama-dev-bounces at openjdk.java.net> on behalf 
>>>> of Sundararajan Athijegannathan 
>>>> <sundararajan.athijegannathan at oracle.com>
>>>> *Sent:* Monday, February 4, 2019 3:24:00 PM
>>>> *To:* 'panama-dev at openjdk.java.net'
>>>> *Subject:* [foreign] RFR 8218281: jextract native library related 
>>>> command line options should be revisited
>>>> Please review.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8218281
>>>> Webrev: https://cr.openjdk.java.net/~sundar/8218281/webrev.00/ 
>>>> <https://cr.openjdk.java.net/%7Esundar/8218281/webrev.00/>
>>>>
>>>> Thanks,
>>>> -Sundar