On Wed, Aug 30, 2017 at 03:55:13PM +0000, Bart Van Assche wrote:
> On Sun, 2017-08-27 at 00:33 +0800, Ming Lei wrote:
> > /**
> > * sbitmap_for_each_set() - Iterate over each set bit in a &struct sbitmap.
> > + * @start: Where to start the iteration
>
> Thanks for having changed the name of this argument ...
>
> > -static inline void sbitmap_for_each_set(struct sbitmap *sb, sb_for_each_fn fn,
> > - void *data)
> > +static inline void __sbitmap_for_each_set(struct sbitmap *sb,
> > + unsigned int start,
> > + sb_for_each_fn fn, void *data)
> > {
> > - unsigned int i;
> > + unsigned int index = SB_NR_TO_INDEX(sb, start);
> > + unsigned int nr = SB_NR_TO_BIT(sb, start);
> > + unsigned int scanned = 0;
>
> ... but I'm still missing a check here whether or not index >= sb->map_nr.
The reason I didn't do that is because the only one user is always to
pass correct 'start'.
OK, will add it.
>
> > - for (i = 0; i < sb->map_nr; i++) {
> > - struct sbitmap_word *word = &sb->map[i];
> > - unsigned int off, nr;
> > + while (1) {
> > + struct sbitmap_word *word = &sb->map[index];
> > + unsigned int depth = min_t(unsigned int, word->depth - nr,
> > + sb->depth - scanned);
> >
> > + scanned += depth;
> > if (!word->word)
> > - continue;
> > + goto next;
> >
> > - nr = 0;
> > - off = i << sb->shift;
> > + depth += nr;
> > + start = index << sb->shift;
>
> The above statement reuses the argument 'start' for a new purpose. This is
> confusing - please don't do this. Why not to keep the name 'off'? That will
> keep the changes minimal.
OK, that will rename the parameter of 'start' as 'off', so that we can
save one local variable.
--
Ming
