[ccan] [PATCH v2 2/2] maskn & bitops: add new modules

[David Gibson]

On Wed, Jul 09, 2014 at 09:49:44PM -0400, Cody P Schafer wrote:
> '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).
> 
> There is some overlap between ilog and bitops that would
> be good to remove (fls).

[snip]
> diff --git a/ccan/bitops/bitops.c b/ccan/bitops/bitops.c
> new file mode 100644
> index 0000000..a9c233b
> --- /dev/null
> +++ b/ccan/bitops/bitops.c
> @@ -0,0 +1,10 @@
> +/* CC0 (Public Domain) - see LICENSE file for details */
> +#include <ccan/bitops/bitops.h>
> +
> +extern inline uintmax_t bit_mask_nz(unsigned bits);
> +extern inline uintmax_t bit_mask(unsigned bits);

Hrm.. does "extern inline" actually have a well-defined portable
meeting?

I still think these functions are so simple they should be static
inlines, at least the builtin versions.  Not everyone has LTO.

> +extern inline unsigned clz_32(uint32_t v);
> +extern inline unsigned ctz_32(uint32_t v);
> +extern inline unsigned fls_32(uint32_t v);
> +extern inline unsigned fls_r1_32(uint32_t v);

As I recall, fls_r1_32 was useful in maskn, but didn't have a
particularly obvious function outside that.  So, wouldn't it make more
sense to be a static in the maskn module itself?

[snip]
> +/**
> + * bit_mask_nz - given a non-zero number of bits, returns a left justified bit
> + *               mask with that many bits set

It's right justified, not left, isn't it?

> + *
> + * Note that the number of bits must be non-zero AND less than or equal to
> + * CHAR_BIT * sizeof(uintmax_t), ie: the number of bits in a uintmax_t.
> + */

I think these descriptive comments would be more accessible in the .h
than the .c.

> +CONST_FUNCTION
> +inline uintmax_t bit_mask_nz(unsigned bits)
> +{
> +	assert(bits);
> +	assert(bits <= (CHAR_BIT * sizeof(uintmax_t)));
> +	return (UINTMAX_C(1) << ((bits) - 1) << 1) - 1;
> +}
> +
> +/**
> + * bit_mask - given a number of bits, returns a left justified bit mask with
> + *            that many bits set
> + *
> + * Note that the number of bits must be less than or equal to CHAR_BIT *
> + * sizeof(uintmax_t), ie: the number of bits in a uintmax_t.
> + */
> +CONST_FUNCTION
> +inline uintmax_t bit_mask(unsigned bits)
> +{
> +	return bits ? bit_mask_nz(bits) : 0;
> +}

This still seems a ludicrously round about way of constructing a
mask.  The -1 << 1 trick pushes the limit on bits out by one by opens
a new problem with bits == 0.  What about

static inline uint uintmax_t bit_mask(unsigned bits)
{
	if (bits >= CHAR_BIT * sizeof(uintmax_t))
		return UINTMAX_C(-1);
	else
		return ~(UINTMAX_MAX << bits);
}

[snip]
> diff --git a/ccan/maskn/LICENSE b/ccan/maskn/LICENSE
> new file mode 120000
> index 0000000..7455044
> --- /dev/null
> +++ b/ccan/maskn/LICENSE
> @@ -0,0 +1 @@
> +../../licenses/LGPL-3
> \ No newline at end of file
> diff --git a/ccan/maskn/_info b/ccan/maskn/_info
> new file mode 100644
> index 0000000..6a71f33
> --- /dev/null
> +++ b/ccan/maskn/_info
> @@ -0,0 +1,44 @@
> +#include "config.h"
> +#include <string.h>
> +#include <stdio.h>
> +
> +/**
> + * maskn - generate masks of N bits for a given range
> + *
> + * This module generates masks of N bits, as opposed to arbitrary masks where
> + * any bits can be 0 or * 1. In this module, only the trailing bits are 0, all
> + * others are 1.
> + *
> + * This mask generation is useful in interfacing with hardware that only
> + * provides limited matching capabilities. An example is the cortex-m3 DWT
> + * hardware module, which can be given a comparison value and a number of bits
> + * to mask off the of anything compared against it.

"off the of"?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/ccan/attachments/20140710/52a8e5b7/attachment.sig>