RFR: 8344079: Minor fixes and cleanups to compiler lint-related code [v2]

[Maurizio Cimadamore]

On Tue, 3 Dec 2024 15:28:34 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

>>> So, one concrete proposal might be:
>>> 
>>> - have LintWarning as a subclass of Warning
>>> - don't change `AbstractLog` - that will still offer a `log` method that takes a `Warning` that can be used for both regular warning and lint warnings (in case the lint warning is not really suppressible)
>> 
>> I'm with you except for these two items. I believe that every warning associated with a lint category is always suppressible at least via the `-Xlint:-foo` command line flag. Do you have any specific counter-examples in mind?
>> 
>> If that's true, then we should be able to go a little further:
>> 
>> 1. Have `LintWarning` be a sibling class, not a subclass, of `Warning`
>> 2. Have `CompilerProperties` reflect the lint categories, by having different nested classes where `LintWarning` factories and fields would appear (one nested class per lint category)
>> 3. Change `AbstractLog` to add methods taking a `LintWarning`; these will automatically add the "[foo]" prefix to the warning text that is emitted
>> 4. Add a convenience instance method `Lint.logIfEnabled()` which accepts a `LintWarning`, does the enablement check, and then (if the category is enabled) invokes the new `Log` method added in the previous step.
>
>> There's also another issue with `logIfEnabled`. While that seems to be the majority of cases, there is a non-trivial amount of uses where the is-enabled check occurs at some outer level, at which point the inner code just warns w/o a check (see all the serial warnings inside `Check` -- but there's several other cases too). So, not sure how much that infrastructure would be worth adding.
> 
> I think it's still worth adding. It's just a convenience method, to be used when it's appropriate - which is in most cases - and not when it's not appropriate. In the inappropriate cases (like within `SerialTypeVisitor`), there's no change - we would just use `log()` like before. But we still accrue the cleanup benefit for most cases.

> 3. Change `AbstractLog` to add methods taking a `LintWarning`; these will automatically add the "[foo]" prefix to the warning text that is emitted

In terms of user experience, what would be the difference between using `log(LintWarning)` and `log(Warning)` ? I believe there is a need for having a method that "just logs" because sometimes (see my previous message) the is-enabled check occurs somewhere else, far from where the diagnostic is logged.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22056#discussion_r1868002845