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

From: Jeff King <peff@peff.net>
To: Christian Couder <christian.couder@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Christian Couder <chriscool@tuxfamily.org>,
	Ramsay Jones <ramsay@ramsayjones.plus.com>,
	Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH v3 3/9] ewah/bitmap: introduce bitmap_word_alloc()
Date: Mon, 9 Dec 2019 01:28:41 -0500	[thread overview]
Message-ID: <20191209062841.GC40570@coredump.intra.peff.net> (raw)
In-Reply-To: <20191115141541.11149-4-chriscool@tuxfamily.org>

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.

>  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).

-Peff