Re: [PATCH] Add "is_power_of_2" checking to log2.h.

[Robert P. J. Day]

On Wed, 31 Jan 2007, Nick Piggin wrote:

> Robert P. J. Day wrote:
> > On Tue, 30 Jan 2007, Nick Piggin wrote:
> >
> >
> > > Robert P. J. Day wrote:
> > >
> > > >  Add the inline function "is_power_of_2()" to log2.h, where the value
> > > > zero is *not* considered to be a power of two.
> > >
> > > > Signed-off-by: Robert P. J. Day <rpjday@xxxxxxxxxxxxxx>
> > > >
> > > > /*
> > > > + *  Determine whether some value is a power of two, where zero is
> > > > + * *not* considered a power of two.
> > > > + */
> > >
> > > Why the qualifier? Zero *is* not a power of 2, is it?
> >
> >
> > no, but it bears repeating since some developers might think it *is*.
> > if you peruse the current kernel code, you'll find some tests of the
> > simpler form:
> >
> > 	((n & (n - 1)) == 0))
> >
> > which is clearly testing for "power of twoness" but which will
> > return true for a value of zero.  that's wrong, and it's why it's
> > emphasized in the comment.
>
> I would have thought you'd comment the broken ones, but that's just
> me.

  good point, so let's just sum up here.  (man, it's hard to believe
that something this simple could drag on so long.  i feel like i'm
discussing free device driver development or something. :-)

  the new is_power_of_2() macro is defined as:

  (n != 0 && ((n & (n - 1)) == 0))

which (correctly, IMHO) does *not* identify zero as a power of two.
if someone truly wants *that* test, they can write it themselves:

  if (x == 0 || is_power_of_2(x))

  this means that, if someone wants to start rewriting those tests in
the source tree, every time they run across an apparent "power of two"
test of the simpler form:

  (n & (n - 1))

they have to ask themselves, "ok, did this coder mean to include zero
or not?"  in some cases, it's probably not going to be obvious.
(maybe the maintainers could do a quick check themselves and make the
substitution 'cuz, once the kernel janitors get ahold of this, you
never know what hilarity will ensue. :-)

  as far as the patch itself i submitted is concerned, the *only*
place that changed the existing semantics was here:

=================================================
--- a/arch/ppc/syslib/ppc85xx_rio.c
+++ b/arch/ppc/syslib/ppc85xx_rio.c
@@ -59,8 +59,6 @@
 #define DBELL_TID(x)           (*(u8 *)(x + DOORBELL_TID_OFFSET))
 #define DBELL_INF(x)           (*(u16 *)(x + DOORBELL_INFO_OFFSET))

-#define is_power_of_2(x)       (((x) & ((x) - 1)) == 0)
-
 struct rio_atmu_regs {
        u32 rowtar;
        u32 pad1;
=================================================

so if the powerpc people are ok with that, then the patch itself
should be fine, and it's only the upcoming substitutions in the source
tree that will have to be checked carefully, one by one.

rday

--

========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://www.fsdev.dreamhosters.com/wiki/index.php?title=Main_Page
========================================================================
-
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/