[PATCH] timekeeping: Use min() to fix Coccinelle warning

[PATCH] timekeeping: Use min() to fix Coccinelle warning

[Thorsten Blum]

Fixes the following Coccinelle/coccicheck warning reported by
minmax.cocci:

	WARNING opportunity for min()

Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
---
 kernel/time/timekeeping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 4e18db1819f8..f1a9c52b7c66 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -799,7 +799,7 @@ static void timekeeping_forward_now(struct timekeeper *tk)
 
 	while (delta > 0) {
 		u64 max = tk->tkr_mono.clock->max_cycles;
-		u64 incr = delta < max ? delta : max;
+		u64 incr = min(delta, max);
 
 		tk->tkr_mono.xtime_nsec += incr * tk->tkr_mono.mult;
 		tk->tkr_raw.xtime_nsec += incr * tk->tkr_raw.mult;
-- 
2.45.2

Re: [PATCH] timekeeping: Use min() to fix Coccinelle warning

[John Stultz]

On Sun, Jun 23, 2024 at 11:24 PM Thorsten Blum <thorsten.blum@toblux.com> wrote:
>
> Fixes the following Coccinelle/coccicheck warning reported by
> minmax.cocci:
>
>         WARNING opportunity for min()
>
> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
> ---
>  kernel/time/timekeeping.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 4e18db1819f8..f1a9c52b7c66 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -799,7 +799,7 @@ static void timekeeping_forward_now(struct timekeeper *tk)
>
>         while (delta > 0) {
>                 u64 max = tk->tkr_mono.clock->max_cycles;
> -               u64 incr = delta < max ? delta : max;
> +               u64 incr = min(delta, max);

Acked-by: John Stultz <jstultz@google.com>

thanks
-john

Re: [PATCH] timekeeping: Use min() to fix Coccinelle warning

[Thomas Gleixner]

Thorsten!

On Mon, Jun 24 2024 at 08:24, Thorsten Blum wrote:

> Fixes the following Coccinelle/coccicheck warning reported by
> minmax.cocci:
>
> 	WARNING opportunity for min()

I'm fine with the change, but not so much with the change log.

You cannot fix a coccinelle warning. You can only fix the code which
triggers the warning, right?

'Opportunity to use min()' is nothing else than an opportunity, but
what's the benefit of replacing correct code with it? What does this
fix?

It fixes nothing. So calling it a fix is confusing at best.

What you want to say is something like this:

  Subject: timekeeping: Replace open coded min()

  Replace open coded min() because $GOOD_REASON

  Discovered by minmax.cocci

$GOOD_REASON is not 'coccinelle emitted a warning'.

Thanks,

        tglx

Re: [PATCH] timekeeping: Use min() to fix Coccinelle warning

[Thorsten Blum]

[Sending this again because editing a draft on my iPhone and then
 sending it from my Mac somehow messed up the message id and didn't show
 up properly on the mailing list. Sorry about the noise.]

Hi Thomas,

On 24. Jun 2024, at 23:36, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, Jun 24 2024 at 08:24, Thorsten Blum wrote:
> 
>> Fixes the following Coccinelle/coccicheck warning reported by
>> minmax.cocci:
>> 
>> WARNING opportunity for min()
> 
> I'm fine with the change, but not so much with the change log.
> 
> You cannot fix a coccinelle warning. You can only fix the code which
> triggers the warning, right?
> 
> 'Opportunity to use min()' is nothing else than an opportunity, but
> what's the benefit of replacing correct code with it? What does this
> fix?
> 
> It fixes nothing. So calling it a fix is confusing at best.

I think it's pretty common to "fix a warning" -- there are thousands of
commits in the kernel using this wording in the summary alone -- even
when the change doesn't actually "fix" anything other than removing the
warning.

However, how about 'resolve' instead?

timekeeping: Use min() to resolve Coccinelle warning

> What you want to say is something like this:
> 
> Subject: timekeeping: Replace open coded min()
> 
> Replace open coded min() because $GOOD_REASON
> 
> Discovered by minmax.cocci
> 
> $GOOD_REASON is not 'coccinelle emitted a warning'.

Removing a warning can be a good reason in itself to refactor code,
because fewer warnings make "real" warnings and potential problems
become more noticeable and thus more likely to get fixed. In short, it
improves maintainability.

To me this is obvious, but I'm happy to add something like "refactor
code to remove warning and improve overall maintainability" to the
commit message.

Thanks,
Thorsten

Re: [PATCH] timekeeping: Use min() to fix Coccinelle warning

[Thomas Gleixner]

Thorsten!

On Sat, Jun 29 2024 at 16:12, Thorsten Blum wrote:
> On 24. Jun 2024, at 23:36, Thomas Gleixner <tglx@linutronix.de> wrote:
>> On Mon, Jun 24 2024 at 08:24, Thorsten Blum wrote:
>> 
>>> Fixes the following Coccinelle/coccicheck warning reported by
>>> minmax.cocci:
>>> 
>>> WARNING opportunity for min()
>> 
>> I'm fine with the change, but not so much with the change log.
>> 
>> You cannot fix a coccinelle warning. You can only fix the code which
>> triggers the warning, right?
>> 
>> 'Opportunity to use min()' is nothing else than an opportunity, but
>> what's the benefit of replacing correct code with it? What does this
>> fix?
>> 
>> It fixes nothing. So calling it a fix is confusing at best.
>
> I think it's pretty common to "fix a warning" -- there are thousands of
> commits in the kernel using this wording in the summary alone -- even
> when the change doesn't actually "fix" anything other than removing the
> warning.

Following the lemmings does not make a technical argument.

> However, how about 'resolve' instead?
>
> timekeeping: Use min() to resolve Coccinelle warning
>
>> What you want to say is something like this:
>> 
>> Subject: timekeeping: Replace open coded min()
>> 
>> Replace open coded min() because $GOOD_REASON
>> 
>> Discovered by minmax.cocci
>> 
>> $GOOD_REASON is not 'coccinelle emitted a warning'.
>
> Removing a warning can be a good reason in itself to refactor code,
> because fewer warnings make "real" warnings and potential problems
> become more noticeable and thus more likely to get fixed. In short, it
> improves maintainability.
>
> To me this is obvious, but I'm happy to add something like "refactor
> code to remove warning and improve overall maintainability" to the
> commit message.

Again. The code _is_ correct and replacing it with min() does neither
fix nor remove a warning.

What's wrong with saying:

       timekeeping: Replace open coded min()

       The open coded min implementation is correct, but triggers a
       Coccinelle warning in minmax.cocci.

       Replace the open coded variant with min() to reduce the noise of
       false positives.

That does not sound as spetacular as 'Fix', but it reflects the actual
technical reason for making this change.

Thanks,

        tglx

Re: [PATCH] timekeeping: Use min() to fix Coccinelle warning

[Thorsten Blum]

Hi Thomas,

On 24. Jun 2024, at 23:36, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, Jun 24 2024 at 08:24, Thorsten Blum wrote:
> 
>> Fixes the following Coccinelle/coccicheck warning reported by
>> minmax.cocci:
>> 
>> WARNING opportunity for min()
> 
> I'm fine with the change, but not so much with the change log.
> 
> You cannot fix a coccinelle warning. You can only fix the code which
> triggers the warning, right?
> 
> 'Opportunity to use min()' is nothing else than an opportunity, but
> what's the benefit of replacing correct code with it? What does this
> fix?
> 
> It fixes nothing. So calling it a fix is confusing at best.

I think it's pretty common to "fix a warning" -- there are thousands of
commits in the kernel using this wording in the summary alone -- even
when the change doesn't actually "fix" anything other than removing the
warning.

However, how about 'resolve' instead?

 timekeeping: Use min() to resolve Coccinelle warning

> What you want to say is something like this:
> 
> Subject: timekeeping: Replace open coded min()
> 
> Replace open coded min() because $GOOD_REASON
> 
> Discovered by minmax.cocci
> 
> $GOOD_REASON is not 'coccinelle emitted a warning'.

Removing a warning can be a good reason in itself to refactor code,
because fewer warnings make "real" warnings and potential problems
become more noticeable and thus more likely to get fixed. In short, it
improves maintainability.

To me this is obvious, but I'm happy to add something like "refactor
code to remove warning and improve overall maintainability" to the
commit message.

Thanks,
Thorsten