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

[Maurizio Cimadamore]

On Tue, 3 Dec 2024 23:55:18 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

>> Not sure I buy your arguments :-) 
>> While the mechanics of lint warning is a bit different, they are _still_ warnings (e.g. the compiler still says `warning: <text>`, and it is still something that counts towards `Werror`. Making them completely separate seems like trying to make them more different than they really are.
>> 
>> And, if we have both `log(Warning)` and `log(LintWarning)` whether they are the same type or not becomes a mot question, because you can still use them in place of another. And, if you have subclassing and have only a single `log(Warning)` method which adds the category or not depending on the _runtime_ type of the warning, then the problem you describe above cannot occur (I think).
>> 
>> So, while I see how one can argue this several ways, the advantage for going down the path you describe seems less clear to me. In terms of object-oriented hierarchy, saying that a lint warning *is a* warning with a category added on top seems to me like a good modelling of the situation we have.
>
> OK I think you're right and it will all work out - using the runtime type ensures the prefix always appears, and the other thing I was worried about, which is someone accidentally logging a lint warning without checking for enablement because they thought it was a generic warning, should be prevented now because the in the code you will see `LintWarnings.FooBar` instead of `Warnings.FooBar` so hopefully it will be very obvious.

I've updated the branch in place. This adds `LintWarnings` in `CompilerProperties` but does *not* add the intermediate grouping based on the lint category. E.g. We have `LintWarnings.DivZero` but not `LintWarnings.DivZero.DivZero`. The reason is twofold:

* Implementation-wise, collecting different warnings associated with the same category starts to be some work in the build tool we use to generate CompilerProperties.
* More importantly, while converting the use cases to use the new `LintWarning` class it seems to me that either the `isEnabled` check is immediately preceding (in which case the new `Lint::logIfEnabled` can be used), or the check is typically far enough that we have no idea on which category we are handling. E.g. pattern like `if (lint) { log ... }` is extremely common, where `lint` is either a method parameter or a class field.

I think I'm fairly happy with where this has landed. Note that the new type `LintWarning` can be used to make the signature of some methods (notably those in `MandatoryWarningHandler`) tighter, so that we can detect more issues at compile-time (and at runtime). In fact, this has helped me fixing a couple of warning keys which were missing the `lint` category in the `compiler.properties` file.

I'm not 100% happy with `Lint::logIfEnabled`: the use sites look good, but having `Lint` keep track of a log seems a bit odd (given that `Lint` objects are duplicated on the fly).

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

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