From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 1 Mar 2025 11:24:52 +0100

Reduce nested max() calls by a single max3() call in this
function implementation.

The source code was transformed by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/xfs/scrub/repair.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index 3b5288d3ef4e..6b23a3943907 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -382,7 +382,7 @@ xrep_calc_ag_resblks(
 			refcbt_sz);
 	xfs_perag_put(pag);

-	return max(max(bnobt_sz, inobt_sz), max(rmapbt_sz, refcbt_sz));
+	return max3(bnobt_sz, inobt_sz, max(rmapbt_sz, refcbt_sz));
 }

 #ifdef CONFIG_XFS_RT
--
2.48.1

On Sat, Mar 01, 2025 at 11:30:52AM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 1 Mar 2025 11:24:52 +0100
> 
> Reduce nested max() calls by a single max3() call in this
> function implementation.
> 
> The source code was transformed by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  fs/xfs/scrub/repair.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index 3b5288d3ef4e..6b23a3943907 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -382,7 +382,7 @@ xrep_calc_ag_resblks(
>  			refcbt_sz);
>  	xfs_perag_put(pag);
> 
> -	return max(max(bnobt_sz, inobt_sz), max(rmapbt_sz, refcbt_sz));
> +	return max3(bnobt_sz, inobt_sz, max(rmapbt_sz, refcbt_sz));

I have nothing against the patch itself, but honestly I don't see how it
improves anything. It boils down to nesting comparison instructions too, and
doesn't make the code more clear IMHO.
So, unless somebody else has a stronger reason to have this change, NAK from my
side.

Carlos

>  }
> 
>  #ifdef CONFIG_XFS_RT
> --
> 2.48.1
>

…
>> +++ b/fs/xfs/scrub/repair.c
>> @@ -382,7 +382,7 @@ xrep_calc_ag_resblks(
>>  			refcbt_sz);
>>  	xfs_perag_put(pag);
>>
>> -	return max(max(bnobt_sz, inobt_sz), max(rmapbt_sz, refcbt_sz));
>> +	return max3(bnobt_sz, inobt_sz, max(rmapbt_sz, refcbt_sz));
> 
> I have nothing against the patch itself, but honestly I don't see how it
> improves anything. It boils down to nesting comparison instructions too, and
> doesn't make the code more clear IMHO.
> So, unless somebody else has a stronger reason to have this change, NAK from my side.
Would you be looking for a wrapper call variant like max4()?

Regards,
Markus

On Wed, Apr 30, 2025 at 10:34:59AM +0200, Markus Elfring wrote:
> …
> >> +++ b/fs/xfs/scrub/repair.c
> >> @@ -382,7 +382,7 @@ xrep_calc_ag_resblks(
> >>  			refcbt_sz);
> >>  	xfs_perag_put(pag);
> >>
> >> -	return max(max(bnobt_sz, inobt_sz), max(rmapbt_sz, refcbt_sz));
> >> +	return max3(bnobt_sz, inobt_sz, max(rmapbt_sz, refcbt_sz));
> >
> > I have nothing against the patch itself, but honestly I don't see how it
> > improves anything. It boils down to nesting comparison instructions too, and
> > doesn't make the code more clear IMHO.
> > So, unless somebody else has a stronger reason to have this change, NAK from my side.
> Would you be looking for a wrapper call variant like max4()?

I have no preference really, between a max(max(), max()) and a max4(a, b, c, d),
the latter is a tad easier to the eyes, if it's worth adding a new max() macro
for that, it's another thing. Although a quick search on the source code
returned returned several usages of the max(max3(a,b,c), d) patterns, so I think
indeed the kernel could benefit of a max4() :)


> 
> Regards,
> Markus