lkml.org 
[lkml]   [2007]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] Add "is_power_of_2" checking to log2.h.
    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@mindspring.com>
    > > > >
    > > > > /*
    > > > > + * 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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/
    \
     
     \ /
      Last update: 2007-01-31 08:39    [W:3.514 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site