RFR: 8345263: Make sure that lint categories are used correctly when logging lint warnings [v2]

[Maurizio Cimadamore]

On Fri, 6 Dec 2024 10:24:03 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>>> we probably need something that will avoid running the lint's code completely in a (semi-)automatic way. That could be part of a more generic 22088.
>> 
>> Agreed.
>> 
>> Slight comment hijack follows...
>> 
>> The [unnecessary suppression warning proposal](https://mail.openjdk.org/pipermail/compiler-dev/2024-November/028573.html) would create a natural answer for this, because there would be a new concept "active" along with "enabled": A category is active if it's enabled OR we are tracking unnecessary suppressions, the category is being suppressed at this point in the code, and so far that suppression is unnecessary (i.e., no warnings would have fired yet).
>> 
>> So linter code would want to check for "active" status (instead of "enabled" status) before doing a non-trivial calculation, which would end up looking like this:
>> 
>> if (lint.isActive(LintCategory.FOO) &&
>>   complicatedCalculationA() &&
>>   complicatedCalculationB()) {
>>     lint.logIfEnabled(log, pos, LintWarnings.FooProblem);
>> }
>> 
>> But perhaps then we should then consider lambda-ification, which would simplify this down to:
>> 
>> lint.check(log, pos, LintWarnings.FooProblem,
>>   () -> complicatedCalculationA() && complicatedCalculationB());
>> 
>> This could be your "(semi-)automatic" option.
>
>> Here, and on a few other places, the checks were ordered so that the potentially more expensive checks were done after the `isEnabled` check. Checking the exact places, I don't think delaying the check is terrible for the cases in this PR, but if we would like to be adding new lints, we probably need something that will avoid running the lint's code completely in a (semi-)automatic way. That could be part of a more generic 22088.
>> 
>> (Admittedly, on some places, the `isEnabled` check was last, after the more expensive checks. This is particularly the case for the `TRY` lint, where the checks might be considerable.)
> 
> I was aware of that. As you might notice, in some cases I preserved the original code structure - in other cases I've changed it when the preceding checks seemed O(1) -- I might have made some mistakes, but that was the spirit.

> But perhaps then we should then consider lambda-ification, which would simplify this down to:
> 
> ```java
> lint.check(log, pos, LintWarnings.FooProblem,
>   () -> complicatedCalculationA() && complicatedCalculationB());
> ```

I thought about this as well but found it a bit too convoluted for my liking. One might already argue that `Lint::logIfEnabled` is doing two things at once (check and log). Making it an open box where you check if the lint is enabled, you check some user provided condition, and if both are true then you log" seems a rather ad-hoc API to add :-)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22553#discussion_r1873003778