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

[Jorn Vernee]

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