RFR: 8344079: Minor fixes and cleanups to compiler lint-related code [v2]
Maurizio Cimadamore
mcimadamore at openjdk.org
Wed Dec 4 17:12:42 UTC 2024
On Wed, 4 Dec 2024 17:09:24 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:
>> 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).
I'll probably issue a PR against that branch, so that we can discuss that there. Sorry for hijacking your PR :-)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22056#discussion_r1869964035
More information about the compiler-dev
mailing list