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

[Jorn Vernee]

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