[ccan] [PATCH] maskn & bitops: add new modules

[Rusty Russell]

Cody P Schafer <dev at codyps.com> writes:
> 'maskn' provides generation of n-bit masks from a given range.
> 'bitops' provides some generic bit operations that maskn needs.
>
> Both operate on 32-bit values only (that's just what I needed at this
> point).
>
> bitops currently has some not-so-great detection of features to support
> the the intrinsics provided by IAR's C compiler for ARM. Not sure how
> we want to handle that. "HAVE_CLZ and HAVE_RBIT" doesn't seem like it
> will be useful for other compilers, and we'll still end up with similar
> ifdefs to generate the builtin_clz/builtin_ctz macros (not to mention
> the need to include intrinsics.h).

Well, we could, but perhaps we could wait until the second user :)

> +#if defined(__GNUC__) || (HAVE_BUILTIN_CLZ && HAVE_BUILTIN_CTZ)

Why the __GNUC__?  If you want the others preferred, perhaps put it first?

> +/* gcc's __builtin_{clz,ctz}() are undefined if v == 0 */
> +# define builtin_clz(v) (v ? __builtin_clz(v) : (sizeof(v) * CHAR_BIT))
> +# define builtin_ctz(v) (v ? __builtin_ctz(v) : (sizeof(v) * CHAR_BIT))
> +#elif defined(__ICCARM__)
> +# include <intrinsics.h>
> +/* emits the ARM instruction, which returns 32 if no bits are set */
> +# define builtin_clz(v) __CLZ(v)
> +# define builtin_ctz(v) __CLZ(__RBIT(v))
> +#endif

> +/* assert(bits > 0) */
> +#define bit_mask_nz(bits) ((UINTMAX_C(1) << ((bits) - 1) << 1) - 1)
> +static inline uintmax_t bit_mask(unsigned bits)
> +{
> +	return bits ? bit_mask_nz(bits) : 0;
> +}

Yech... The double shift really deserves a comment (this bit me on
powerpc once).

And it doesn't help if you do something silly like: bit_mask(65).

I prefer bitmask_32 and bitmask_64, to match the rest.  Then:

static inline uint32_t bitmask_32(unsigned bits)
{
        BUILD_ASSERT(bits <= 32);
        assert(bits <= 32);

        /* Shift by 32 is undefined, so shift by 31 then 1 */
        return bits ? ((1U << (bits - 1)) << 1) - 1 : 0;
}

> +unsigned fls_m1_32(uint32_t v) CONST_FUNCTION;

OK, I have no idea what this does :(

> +unsigned clz_32(uint32_t v) CONST_FUNCTION;
> +unsigned ctz_32(uint32_t v) CONST_FUNCTION;

Really needs documentation :)

Cheers,
Rusty.