On Tue, 6 Mar 2007, David Howells wrote:
> /**
> + * ilog2_up - rounded up log of base 2 of 32-bit or a 64-bit unsigned value
> + * @n - parameter
> + *
> + * constant-capable log of base 2 calculation
> + * - this can be used to initialise global variables from constant data,
> hence
> + * the massive ternary operator construction
> + * - the result is rounded up
> + * - the result is undefined when n < 1
> + *
> + * selects the appropriately-sized optimised version depending on sizeof(n)
> + */
> +#define ilog2_up(n) ((n) == 1 ? 0 : ilog2((n) - 1) + 1)
This is wrong. It uses "n" twice, which makes it unsafe as a macro.
It would need to be an inline function, but then the global initializer
comment is wrong.
Or it could use a "__builtin_constant_p()" (which gcc defines to not have
side effects) to allow the multiple use for constant data.
Or we could require that "ilog2(0)" returns -1, and then we could just say
#define ilog2_up(n) (ilog2((n)-1)+1)
Or.. ?
The whole "get_order()" macro also has some serious lack of parenthesis.
In general, commit 39d61db0edb34d60b83c5e0d62d0e906578cc707 just was
pretty damn bad!
I'm becoming a bit disgruntled about this whole thing, I have to admit.
I'm just not sure the bugs here are worth it. Especially considering that
__get_order() has apparently never even tested these things to begin with,
since nobody but FRV has ever #defined the ARCH_HAS_ILOG2_U?? macros.
The whole *reason* for that mess seems to be bogus too, since at least
ia64 still has its own inline "get_order()", which means that nobody can
use get_order() for constant initializers *anyway*, quite unlike the
comments say.
So the whole thing is:
- buggy
- untested
- has untrue comments
- makes no real sense
and I'm inclined to just revert 39d61db0 instead of adding more and more
breakage to it, since it's simply not going to help with the fundamental
problems!
Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
