lkml.org 
[lkml]   [2015]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 1/3] lib: find_*_bit reimplementation
This basically has my Reviewed-by: (I'll send it in a few hours
when I have time to do a real final copy-editing), but a few minor
notes:

>+ /*
>+ * This is an equvalent for:
>+ *
>+ * tmp = find_set ? addr[start / BITS_PER_LONG]
>+ * : ~addr[start / BITS_PER_LONG];
>+ *
>+ * but saves a branch condition.
>+ *
>+ * Thanks George Spelvin <linux@horizon.com> for idea.
>+ */
>+ tmp = addr[start / BITS_PER_LONG] ^ mask;

1. There's no need for detailed credit for such a trivial and obvious
thing. If you want to comment it, describe the use of the parameter
in the function header, e.g.

+/*
+ * "mask" is either 0 or ~0UL and XORed into each fetched word, to select between
+ * searching for one bits and zero bits. If this function is inlined, GCC is
+ * smart enough to propagate the constant.
+ */

2. The variable might be more meaningfully named "xor" or "invert"; "mask"
suggests something that is &-ed with a value.


> unsigned long find_next_zero_bit(const unsigned long *addr, unsigned long size,
> unsigned long offset)
> {
>+ return _find_next_bit(addr, size, offset, ~0UL); <---
> }

> unsigned long find_first_zero_bit(const unsigned long *addr, unsigned long size)
> {
>+ unsigned long idx;
>
>+ for (idx = 0; idx * BITS_PER_LONG < size; idx++) {
>+ if (addr[idx] != ULONG_MAX) <---
>+ return min(idx * BITS_PER_LONG + ffz(addr[idx]), size);
> }
>
>+ return size;
> }

3. Using two names (ULONG_MAX and ~0UL) for the same thing is a bit odd;
you might want to be consistent.

I'll ack it either way; none of these are significant technical issues.


\
 
 \ /
  Last update: 2015-02-08 20:01    [W:0.174 / U:6.880 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site