Re: [PATCH] fix round_up/down

[Andrew Morton]

On Wed, 01 Jul 2009 12:39:35 -0700
Joe Perches <joe@xxxxxxxxxxx> wrote:

> On Wed, 2009-07-01 at 12:33 -0700, Yinghai Lu wrote:
> > --- linux-2.6.orig/include/linux/kernel.h
> > +++ linux-2.6/include/linux/kernel.h
> []
> >  #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
> > +#define rounddown(x, y) (((x) / (y)) * (y))
> []
> > +#define round_up(x,y) ((((x)-1) | __round_mask(x,y))+1)
> > +#define round_down(x,y) ((x) & ~__round_mask(x,y))
> 
> Isn't this just asking for trouble?

Yes, I think so.  round_up() versus roundup() is a bit subtle!

> How about a better name?
> Maybe masked_roundup, or roundup_ala_zorro...

Yes.  roundup() wasn't a well-chosen identifier, really.  But I guess
it's compatible with the faster bitwise-based rounding operation so
it's OK to have a special fast version of roundup() for the cases we're
rounding up to a power-of-2.


umm, how about roundup_pow2()?  Sucks?


Also, it it lower-case or all-caps?  I think it should be all-caps.  Because

a) it is a macro, and it can ONLY be implemented as a macro, so
   there's no point in pretending that it might be a C function and that the
   caller needn't care.

b) several of these macros evaluate their args multiple times and
   hence will produce buggy or inefficeint code when passed expressions
   with side-effects.  So we should warn people that these things are macros.

   We should also fix that, dammit.  The proposed new rounddown()
   above has this failing as well.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/