Re: [PATCH v2 1/2] util_macros.h: fix/rework find_closest() macros

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 6 Nov 2024 16:03:36 +0200 Alexandru Ardelean <aardelean@baylibre.com> wrote:

> On Wed, Nov 6, 2024 at 1:08 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> > Can we fix both issues by just giving up on the macro approach and
> > reimplement them in out-of-line C code?  All the sites I looked at are
> > using 32-bit quantities - a mix of signed and unsigned.
> >
> 
> Converting this to a static-inline was my other thought, rather than
> keeping the macros.

Non-inline, I think.  It's big.

> But I'm not sure where to draw the line between too much rework vs a bug-fix.
> Just fixing the bug was done in V1 of this patch, but then the kunit
> exposed a bunch more.

Sure, just the minimum for a bugfix.

> > It's separate from this bugfix of course, but would it be feasible for
> > someone to go switch all callers to use u32's then reimplement these in
> > lib/find_closest.c?
> >
> 
> That would work.
> How would a rework be preferred?
> As a continuation to this patchset? Or a V3 to this patchset?

A new and separate patchset.  A low-priority cleanup from whoever has
the time and motivation ;)

> But, moving forward: what would some preferences be?
> - have variants of find_closest() for unsigned/signed arrays? (
> find_closest_u32() or find_closest_i32() ?)
>    - AFAICT so far, there aren't any values in the arrays that get
> close to INT32_MAX, so int32 may work for now
>    - maybe later some 64-bit variants could be added if needed
> - should the variables X, mid, left & right be the same signedness as the array
>
> The only preference (towards which I'm leaning) is just making sure
> that X (and friends) are signed.

Yes, I guess int32 would be best.  I agree that unsigned values greater
than INT_MAX are unlikely.

I suggest a series of patches which convert individual callers to int32
and the final patch introduces lib/find_closest.c.