Re: [PATCH v3 3/9] ewah/bitmap: introduce bitmap_word_alloc()

From: Christian Couder <christian.couder@gmail.com>
Date: Dec 11, 2019, 13:50 UTC
Message-ID: <CAP8UFD3PwuSyJ0kjiYKo_UbgqzC1hjJS0rkBjhdfuNmC6hvGvw@mail.gmail.com>
In-Reply-To: <20191209062841.GC40570@coredump.intra.peff.net>

On Mon, Dec 9, 2019 at 7:28 AM Jeff King <peff@peff.net> wrote:

Show 12 quoted lines

>
> On Fri, Nov 15, 2019 at 03:15:35PM +0100, Christian Couder wrote:
>
> > In a following commit we will need to allocate a variable
> > number of bitmap words, instead of always 32, so let's add
> > bitmap_word_alloc() for this purpose.
>
> This name is much better than the horrible "bitmap_new2()" it was called
> in the original. ;)
>
> I wonder if "new_with_world_alloc" or "new_with_size" would make it more
> obvious that this is also a constructor, though.

bitmap_new_with_word_alloc() feels a bit verbose to me for such a short function, so for now I kept bitmap_word_alloc().

I think that if we really want to use "new" for constructors then a better solution would be something like renaming bitmap_new(void) to bitmap_new_default(void) and bitmap_word_alloc(size_t word_alloc) to bitmap_new(size_t word_alloc).

Show 15 quoted lines

> >  void bitmap_set(struct bitmap *self, size_t pos)
> >  {
> >       size_t block = EWAH_BLOCK(pos);
> >
> >       if (block >= self->word_alloc) {
> >               size_t old_size = self->word_alloc;
> > -             self->word_alloc = block * 2;
> > +             self->word_alloc = block ? block * 2 : 1;
>
> Since this hunk caused so much confusion, maybe worth calling it out in
> the commit message. Something like:
>
>   Note that we have to adjust the block growth in bitmap_set(), since
>   a caller could now use an initial size of "0" (we don't plan to do
>   that, but it doesn't hurt to be defensive).

Ok, I added the above as a commit message note in my current version.