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

[Sundararajan Athijegannathan]

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