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

[Archie Cobbs]

On Wed, 4 Dec 2024 17:10:11 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> 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).
>
> I'll probably issue a PR against that branch, so that we can discuss that there. Sorry for hijacking your PR :-)

I think this sounds like a good plan and agree with your reasoning for not over-complicating the class hierarchy.

I'm sure I'll have some minor comments on the patch but will wait for you to create a new PR and comment there when it appears.

Thanks.

> 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).

I was thinking that a `log` parameter would have to be passed as a parameter, because there is at least one subclass somewhere (`ReusableLog extends Log`), but maybe I was being too conservative.

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

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