Messages in this thread | | | Date | 8 Feb 2015 13:48:23 -0500 | From | "George Spelvin" <> | Subject | Re: [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.
| |