On Wed, 4 Dec 2024 19:11:03 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org>
wrote:
>> This PR tightens up the logic by which javac reports lint warnings.
>> Currently, lint warnings are typically emitted using the following idiom:
>>
>>
>> if (lint.isEnabled(LintCategory.DIVZERO) {
>> log.warning(LintCategory.DIVZERO, pos, Warnings.DIVZERO);
>> }
>>
>> There are some issues with this approach:
>>
>> * logging a lint warning has to be preceded by the correct `isEnabled` check
>> * the check and the `log::warning` call must share the same `LintCategory`
>> * the selected warning key in the `Warnings` class must also make sense for
>> the selected `LintCategory`
>>
>> This PR addresses these issues, so that the above code is now written as
>> follows:
>>
>>
>> lint.logIfEnabled(pos, LintWarnings.DIVZERO);
>>
>>
>> The new idiom builds on a number of small improvements:
>>
>> * the lint category is now tracked directly in the `compiler.properties`
>> file;
>> * a new `LintWarning` class is added to `JCDiagnostic` to model a warning
>> key that is also associated with a speicfic `LintCategory` enum constant;
>> * the `CompilerProperties` class has a new group of compiler keys, nested in
>> the new `LintWarnings` class. This class defines the `LintWarning` objects
>> for all the warning keys in `compiler.properties` that have a lint category
>> set
>> * A new method `Lint::logIfEnabled(Position, LintWarning)` is added - which
>> simplifies the logging of lint warnings in many common cases, by merging the
>> `isEnabled` check together with the logging.
>>
>> As bonus points, the signatures of some methods in `Check` and
>> `MandatoryWarningHandler` have been tightened to accept not just a
>> `Warning`, but a `LintWarning`. This makes it impossible, for instance, to
>> use `Check::warnUnchecked` with a warning that is not a true lint warning.
>>
>> Many thanks @archiecobbs for the useful discussions!
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java line 398:
>
>> 396: */
>> 397: public void logIfEnabled(DiagnosticPosition pos, LintWarning
>> warning) {
>> 398: if (isEnabled(warning.getLintCategory())) {
>
> I'm not 100% sure about this method. On the one hand it makes clients simpler
> - but it does require `Lint` keeping track of a `log`. An alternative could
> be to add a method in `AbstractLog`, e.g.:
>
> warningIfEnabled(DiagnosticPosition pos, Lint lint, LintWarning key)
elsewhere, @archiecobbs suggested to maybe make `log` a parameter of the
method. I like the simplicity of the suggestion, and I will go with it.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22553#discussion_r1870330363
