From: Carl Worth <cworth@cworth.org>
To: Austin Clements <amdragon@MIT.EDU>
Subject: Re: [PATCH] Various small clean-ups to doc ID set code.
In-Reply-To: <20101208220153.GT2447@mit.edu>
References: <20101117192826.GU2439@mit.edu>
	<874oap5aek.fsf@yoom.home.cworth.org>
	<20101208215844.GS2447@mit.edu> <20101208220153.GT2447@mit.edu>
User-Agent: Notmuch/0.5 (http://notmuchmail.org) Emacs/23.2.1
	(i486-pc-linux-gnu)
Date: Sat, 29 Jan 2011 07:36:13 +1000
Message-ID: <8762t8n22a.fsf@yoom.home.cworth.org>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha1; protocol="application/pgp-signature"
Cc: notmuch@notmuchmail.org
Precedence: list

--=-=-=
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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 =3D 0; i < arr->len; i++)
> +	max =3D 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 >=3D 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.

=2DCarl

=2D-=20
carl.d.worth@intel.com

--=-=-=
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iD8DBQFNQzbN6JDdNq8qSWgRAvgBAJ42F8FC3jWC+TP6LrTIHj/aihdZYACeLuNL
dZ7fhl3lzHBTsU6u56fBAJ4=
=z68Z
-----END PGP SIGNATURE-----
--=-=-=--