On Wed, 8 Dec 2010 17:01:53 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Remove the repeated "sizeof (doc_ids->bitmap[0])" that bothered cworth
> by instead defining macros to compute the word and bit offset of a
> given bit in the bitmap.
> 
> Don't require the caller of _notmuch_doc_id_set_init to pass in a
> correct bound; instead compute it from the array.  This simplifies the
> caller and makes this interface easier to use correctly.
...
> +#define BITMAP_WORD(bit) ((bit) / sizeof (unsigned int))
> +#define BITMAP_BIT(bit) ((bit) % sizeof (unsigned int))

These macros look great, they definitely simplify the code.

>  _notmuch_doc_id_set_init (void *ctx,
>  			  notmuch_doc_id_set_t *doc_ids,
> -			  GArray *arr, unsigned int bound)
> +			  GArray *arr)
...
> +    for (unsigned int i = 0; i < arr->len; i++)
> +	max = MAX(max, g_array_index (arr, unsigned int, i));

And computing an argument automatically definitely makes the interface
easier to use. So that's good too. But those two changes are independent
so really need to be in separate commits.

> -    if (doc_id >= doc_ids->bound)
> +    if (doc_id > doc_ids->max)

And this looks really like a *third* independent change to me.

A code change like the above has the chance to introduce (or fix) an
off-by-one bug---or even leave the code effectively unchanged as the
intent is here.

In order to distinguish all of those cases, I'd like to see a change
like this as a minimal change, and described in the commit
message. (Rather than hidden amongst "various cleanups" that are mostly
about replacing some common code with a macro.)

So I'd be happy to see this patch broken up and sent again.

-Carl

-- 
carl.d.worth@intel.com