Re: [PATCH] bitmap: fix n__ signess

[Elliott Mitchell <ehem+xen@xxxxxxx>]

On Fri, Sep 29, 2023 at 02:13:59PM -0700, Stefano Stabellini wrote:
> On Fri, 29 Sep 2023, Julien Grall wrote:
> > On 29/09/2023 00:19, Stefano Stabellini wrote:
> > > All callers of the bitmap_switch macro (which are all within bitmap.h)
> > > pass an int as first parameter. Do not assign it to an unsigned int
> > > potentially introducing errors.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> > > ---
> > > 
> > > We could also have gone the other way and change all the callers and the
> > > callers' callers to use an unsigned int instead, but I went for the path
> > > of least resistance.
> > 
> > I understand this will solve the issue right now because the callers are all
> > passing 'int'. However, all the callers will need to switch to 'unsigned 
> > int'
> > in order to solve violations in their callers.
> > 
> > That unless we decide to use 'int' everywhere, but I think this is a bad 
> > idea
> > because 'n__' is not supposed to be negative.
> > 
> > Overall, this may be an easy win right now, but this will need to be 
> > reverted.
> > So, I am not happy to ack it and would in fact be leaning towards Nacking 
> > it.
> 
> I understand this point and I was undecided myself about the approach.
> The issue for me is the overwhelming amount of gcc warnings (thankfully
> Luca's script helps a lot with it). With so many warning, it is
> difficult to draw the line where to stop fixing things to generate a
> digestable patch and not having the feeling of unraveling an infinite
> ball of yarn. So, worried about having to change hundreds of lines of
> code, I submitted the minimal change instead.
> 
> In this case though unsigned int is obviously the right type and the
> patch below works as well. So I think that's better.

This looks much better to me.  Did changing the nbits arguments of
bitmap_copy(), bitmap_fill(), and bitmap_weight(), result in the mess
getting worse?  If so then this is a reasonable stopping point, but if
you didn't check I would be tempted to do so (ensure consistency amoung
these functions).


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@xxxxxxx  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445