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

[Maurizio Cimadamore]

Now THAT'S a Logger :-) :-)

Great piece of work.

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

* TypeDictionary - is that debug info not important? (just asking)


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