Messages in this thread | | | Date | Tue, 10 May 2022 12:11:27 -0700 | From | Yury Norov <> | Subject | Re: [PATCH 08/22] bitops: introduce MANY_BITS() macro |
| |
On Tue, May 10, 2022 at 05:54:13PM +0000, David Laight wrote: > From: Alexei Starovoitov > > Sent: 10 May 2022 17:51 > ... > > +/* Return: nonzero if 2 or more bits are set */ > > +#define MANY_BITS(n) ((n) & ((n) - 1)) > > You can't have a macro that expands its argument twice.
Yes, I'll fix it. > ... > > > static inline __attribute__((const)) > > > bool is_power_of_2(unsigned long n) > > > { > > > - return (n != 0 && ((n & (n - 1)) == 0)); > > > + return n != 0 && !MANY_BITS(n); > > > } > > > > Please don't. Open coded version is much easier to read.
To me the human-readable version is easier to read. Still, if you thing that n & (n - 1) is simpler, what for this function is needed at all?
> Especially if you remove all the spare parenthesis. > return n && !(n & (n - 1)); > > I bet a lot of callers know the value is non-zero. > > I suspect you'll find at least one caller that uses > is_power_of_2() assuming it is !(n & (n - 1)) and > so is wrong for zero. Another thing is that despite __attribute__(const), gcc sometimes doesn't recognize it as constant expression, and people have to workaround it. XTENSA is the example for 1st case, and for the 2nd:
arch/powerpc/mm/init-common.c: unsigned long minalign = max(MAX_PGTABLE_INDEX_SIZE + 1, HUGEPD_SHIFT_MASK + 1);
/* It would be nice if this was a BUILD_BUG_ON(), but at the * moment, gcc doesn't seem to recognize is_power_of_2 * as a constant expression, so so much for that. */ BUG_ON(!is_power_of_2(minalign));
This convinced me that we need a simple macro that is decoupled with pow_2 semantics and can be used in another macros like BUILD_BUG_ON().
Thanks, Yury
| |