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

[Maurizio Cimadamore]

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