Re: [PATCH] fix round_up/down

Andrew Morton
2009-07-01T20:04:37Z

On Wed, 01 Jul 2009 12:39:35 -0700
Joe Perches <joe@perches.com> 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.