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

[Cody P Schafer]

On Thu, Jul 10, 2014 at 7:08 AM, David Gibson
<david at gibson.dropbear.id.au> wrote:
> 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.
[...]
>> 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?

In c99 and later, yes. The c99 rationale (
http://www.open-std.org/jtc1/sc22/wg14/www/C99RationaleV5.10.pdf ) has
some details, which this stack overflow post (
http://stackoverflow.com/a/6312854 ) has a textual copy of.

c89, on the other hand, lacks inline and gnu89 reverses the meaning of
"extern inline" and "inline".

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

Well, I don't want these to be unconditionally inlined. I'd very much
rather have the compiler decide than increase code size even when it
isn't a great idea.

>> +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?

I don't really see a point in hiding it. Someone else may have an
algorithm it's useful in.

>> +/**
>> + * 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?

ya, you're correct, it is right justified.

>> + *
>> + * 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.

This is the header. I could move them from the definitions to the
declarations, though. Perhaps move the definitions to a seperate
header to make clear what the api is.

>> +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]

I'm not opposed to swapping it out. Defining the function for all
inputs certainly has some benefits.

>> 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"?
>

"off the lower bits of"