[foreign] RFR 8219932: Split jextract's Context into multiple (immutable) sub-components

[Maurizio Cimadamore]

Looks good to me.

Thanks for pointing me at the TypeDefHandler changes, I did not notice 
that on the earlier pass.

It is also much better to report diagnostic there, rather than on the 
dictionary - this connects to what I was saying the other day (when 
discussing logging inside of the struct layout generator).

Push it!

Maurizio

On 01/03/2019 17:54, Jorn Vernee wrote:
> Maurizio Cimadamore schreef op 2019-03-01 17:52:
>> Now THAT'S a Logger :-) :-)
>>
>> Great piece of work.
>
> Thanks! :-)
>
>> Minor comments:
>>
>> * HeaderResolver  should use Log, not access to underlying logger
>> (presumably to call 'config' - but that can be done with the general
>> Log::print, no?)
>>
>> * JModWriter, JarWriter, TypedefHandler, Parser - same as above - I
>> suggest make logger private in Log, and make sure all clients just use
>> Log
>
> I was wondering about this as well, and whether to put in convenience 
> methods for all the different log levels or not. I'll at least add an 
> overload for Log::print that takes a Supplier<String> for the message; 
> I like that it can be used to visually group the code related to the 
> logging e.g.:
>
> ```
> logger.print(Level.INFO, () -> {
>             StringBuilder sb = new StringBuilder(
>                     "Parsing header file " + path + " with following 
> args:\n");
>             int i = 0;
>             for (String arg : args) {
>                 sb.append("arg[");
>                 sb.append(i++);
>                 sb.append("] = ");
>                 sb.append(arg);
>                 sb.append("\n");
>             }
>             return sb.toString();
>         });
> ```
>
>> * TypeDictionary - is that debug info not important? (just asking)
>
> Yes, but we can't easily get a Log instance at that point, so I 
> created a similar debug print in TypedefHandler instead.
>
> Updated webrev: 
> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8219932/webrev.02/
>
> Jorn
>
>>
>> Maurizio
>>
>>
>>
>> On 01/03/2019 16:09, Jorn Vernee wrote:
>>> I think your suggestions are very good. I had been thinking about 
>>> encapsulating Log::out and Log::err more as well, but held off on it 
>>> because I didn't want to put too many changes into the same patch. 
>>> I'm happy to add those suggestions though :)
>>>
>>> While implementing this I found out that the Logger in Log was not 
>>> actually being used by anything. I thought about removing it, but 
>>> since Logger already has functionality for handling log levels I 
>>> instead changed the implementation of Log to depend on the logger 
>>> instead, and wrapped the err PrintWriter in a logger handler.
>>>
>>> I also added a printStackTrace method to Log for printing 
>>> stacktraces, which is guarded by Main.DEBUG.
>>>
>>> One of the bigger things was that a bunch of classes were creating 
>>> their own Loggers, but these were not being affected by the --log 
>>> flag passed to jextract AFAICT. I changed all of those to use the 
>>> Logger from Log as well, so we can control the log level of all the 
>>> loggers at once by changing the log level passed to Log. (There was 
>>> 1 use of Logger in TypeDictionary, which didn't have a Context 
>>> nearby, so I moved it to TypedefHandler instead).
>>>
>>> When updating all the code referencing the old Loggers I also took 
>>> the opportunity to remove any unused imports.
>>>
>>> I was almost able to get rid of the Log::err and Log::out 
>>> PrintWriters completely, but these were being passed to the Jmod 
>>> tool at some point, so I've kept them for now.
>>>
>>> Updated webrev: 
>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8219932/webrev.01/
>>>
>>> So, there are currently 2 ways of logging; 1. Through 
>>> Log::printError/Warning/Note which expect a message ID from the 
>>> resource file. 2. Through the central java.util.Logger. Maybe we'd 
>>> want to keep only one or the other? But, I wanted to get some 
>>> feedback at this point.
>>>
>>> Cheers,
>>> Jorn
>>>
>>> Maurizio Cimadamore schreef op 2019-02-28 17:27:
>>>> Very nice, I like it. You manage to keep things relatively simple,
>>>> without over-engineering things too much, which is always hard with
>>>> these kind of cleanups.
>>>>
>>>> My #1 request is that we encapsulate logging methods a bit more. I
>>>> don't like that clients have to (morally) do:
>>>>
>>>> Log.stderr.println(Log.format(...))
>>>>
>>>> It feels like 'println' should be an instance method of Log.
>>>>
>>>> Note also that jextract always seems to use stderr for reporting
>>>> diagnostics, so Log::println should probably use that (and keep
>>>> Log::out around in case you need it).
>>>>
>>>> Number #2 request, also Log-related, is that not all diagnostics are
>>>> created equals - some are errors we should always report, other are
>>>> warnings, other are notes that are only enabled in debugging mode. It
>>>> would be nice if Log had different methods for these:
>>>>
>>>> printError
>>>> printWarning
>>>> printNote
>>>>
>>>> which then allows us to do centralized filtering of the diagnostic
>>>> that are reported by jextract.
>>>>
>>>> Maurizio
>>>>
>>>>
>>>> On 28/02/2019 16:02, Jorn Vernee wrote:
>>>>> Hi,
>>>>>
>>>>> Continuing this from an earlier thread [1]. Here is the first 
>>>>> iteration of the ideas that were discussed.
>>>>>
>>>>> This patch splits up Context into multiple sub-components. These are:
>>>>>
>>>>> - Options, a collection of options for a jextract run.
>>>>> - Log, logging PrintWriters + Logger
>>>>> - A List<Path> with jextract source (.h) files.
>>>>>
>>>>> Context is then used as an argument to a particular jextract run.
>>>>>
>>>>> I've held off on adding JextractTool as a sub-component as well, 
>>>>> since that would create a circular dependency, and I'm not sure 
>>>>> what the advantage of adding it would be at this time, since it's 
>>>>> not accessed by other internal parts of the jextract run.
>>>>>
>>>>> I've moved some functions to other classes, where I felt they made 
>>>>> more sense;
>>>>>
>>>>> - Main.format -> Log.format
>>>>> - Main.getBuiltinHeadersDir -> Context.getBuiltinHeadersDir
>>>>> - Context.findLibraryPath -> Utils.findLibraryPath
>>>>>
>>>>> To support the immutability of the Options sub-component I created 
>>>>> a builder for it, and moved the code that creates it to a separate 
>>>>> method in Main. In some cases the code wanted to exit early and 
>>>>> return an exit code, this is now replaced with throwing a custom 
>>>>> exception, which is caught in the `int run(String[] args)` method.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8219932
>>>>> Webrev: 
>>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8219932/webrev.00/
>>>>>
>>>>> Thanks,
>>>>> Jorn
>>>>>
>>>>> [1] : 
>>>>> https://mail.openjdk.java.net/pipermail/panama-dev/2019-February/004627.html