48/f5678fabeb32dd72dbefed16e672b3867da1e6

   1 Return-Path: <cworth@cworth.org>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id 970CB429E22\r
   6         for <notmuch@notmuchmail.org>; Fri, 28 Jan 2011 14:02:53 -0800 (PST)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -0.99\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.99 tagged_above=-999 required=5\r
  12         tests=[ALL_TRUSTED=-1, T_MIME_NO_TEXT=0.01] autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id NHVzgcad1a9x; Fri, 28 Jan 2011 14:02:52 -0800 (PST)\r
  16 Received: from yoom.home.cworth.org (localhost [127.0.0.1])\r
  17         by olra.theworths.org (Postfix) with ESMTP id EAA99429E20;\r
  18         Fri, 28 Jan 2011 14:02:50 -0800 (PST)\r
  19 Received: by yoom.home.cworth.org (Postfix, from userid 1000)\r
  20         id ED09925449D; Sat, 29 Jan 2011 07:36:13 +1000 (EST)\r
  21 From: Carl Worth <cworth@cworth.org>\r
  22 To: Austin Clements <amdragon@MIT.EDU>\r
  23 Subject: Re: [PATCH] Various small clean-ups to doc ID set code.\r
  24 In-Reply-To: <20101208220153.GT2447@mit.edu>\r
  25 References: <20101117192826.GU2439@mit.edu>\r
  26         <874oap5aek.fsf@yoom.home.cworth.org>\r
  27         <20101208215844.GS2447@mit.edu> <20101208220153.GT2447@mit.edu>\r
  28 User-Agent: Notmuch/0.5 (http://notmuchmail.org) Emacs/23.2.1\r
  29         (i486-pc-linux-gnu)\r
  30 Date: Sat, 29 Jan 2011 07:36:13 +1000\r
  31 Message-ID: <8762t8n22a.fsf@yoom.home.cworth.org>\r
  32 MIME-Version: 1.0\r
  33 Content-Type: multipart/signed; boundary="=-=-=";\r
  34         micalg=pgp-sha1; protocol="application/pgp-signature"\r
  35 Cc: notmuch@notmuchmail.org\r
  36 X-BeenThere: notmuch@notmuchmail.org\r
  37 X-Mailman-Version: 2.1.13\r
  38 Precedence: list\r
  39 List-Id: "Use and development of the notmuch mail system."\r
  40         <notmuch.notmuchmail.org>\r
  41 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  42         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  43 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  44 List-Post: <mailto:notmuch@notmuchmail.org>\r
  45 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  46 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  47         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  48 X-List-Received-Date: Fri, 28 Jan 2011 22:02:54 -0000\r
  49 \r
  50 --=-=-=\r
  51 Content-Transfer-Encoding: quoted-printable\r
  52 \r
  53 On Wed, 8 Dec 2010 17:01:53 -0500, Austin Clements <amdragon@MIT.EDU> wrote:\r
  54 > Remove the repeated "sizeof (doc_ids->bitmap[0])" that bothered cworth\r
  55 > by instead defining macros to compute the word and bit offset of a\r
  56 > given bit in the bitmap.\r
  57 >=20\r
  58 > Don't require the caller of _notmuch_doc_id_set_init to pass in a\r
  59 > correct bound; instead compute it from the array.  This simplifies the\r
  60 > caller and makes this interface easier to use correctly.\r
  61 ...\r
  62 > +#define BITMAP_WORD(bit) ((bit) / sizeof (unsigned int))\r
  63 > +#define BITMAP_BIT(bit) ((bit) % sizeof (unsigned int))\r
  64 \r
  65 These macros look great, they definitely simplify the code.\r
  66 \r
  67 >  _notmuch_doc_id_set_init (void *ctx,\r
  68 >                         notmuch_doc_id_set_t *doc_ids,\r
  69 > -                       GArray *arr, unsigned int bound)\r
  70 > +                       GArray *arr)\r
  71 ...\r
  72 > +    for (unsigned int i =3D 0; i < arr->len; i++)\r
  73 > +     max =3D MAX(max, g_array_index (arr, unsigned int, i));\r
  74 \r
  75 And computing an argument automatically definitely makes the interface\r
  76 easier to use. So that's good too. But those two changes are independent\r
  77 so really need to be in separate commits.\r
  78 \r
  79 > -    if (doc_id >=3D doc_ids->bound)\r
  80 > +    if (doc_id > doc_ids->max)\r
  81 \r
  82 And this looks really like a *third* independent change to me.\r
  83 \r
  84 A code change like the above has the chance to introduce (or fix) an\r
  85 off-by-one bug---or even leave the code effectively unchanged as the\r
  86 intent is here.\r
  87 \r
  88 In order to distinguish all of those cases, I'd like to see a change\r
  89 like this as a minimal change, and described in the commit\r
  90 message. (Rather than hidden amongst "various cleanups" that are mostly\r
  91 about replacing some common code with a macro.)\r
  92 \r
  93 So I'd be happy to see this patch broken up and sent again.\r
  94 \r
  95 =2DCarl\r
  96 \r
  97 =2D-=20\r
  98 carl.d.worth@intel.com\r
  99 \r
 100 --=-=-=\r
 101 Content-Type: application/pgp-signature\r
 102 \r
 103 -----BEGIN PGP SIGNATURE-----\r
 104 Version: GnuPG v1.4.10 (GNU/Linux)\r
 105 \r
 106 iD8DBQFNQzbN6JDdNq8qSWgRAvgBAJ42F8FC3jWC+TP6LrTIHj/aihdZYACeLuNL\r
 107 dZ7fhl3lzHBTsU6u56fBAJ4=\r
 108 =z68Z\r
 109 -----END PGP SIGNATURE-----\r
 110 --=-=-=--\r