Date: Sun, 3 Feb 2013 15:01:27 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Robert Mast <beheerder@tekenbeetziekten.nl>
Subject: Re: [PATCH] This patch is a little finger excercise for working with
	git. I found a piece of code that I didn't understand at first.
Message-ID: <20130203200127.GM4445@mit.edu>
References: <1359917491-17178-1-git-send-email-beheerder@tekenbeetziekten.nl>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1359917491-17178-1-git-send-email-beheerder@tekenbeetziekten.nl>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

If I'm reading this right, this patch reduces a 8x memory waste to a
4x memory waste, but doesn't actually eliminate the waste.

Quoth Robert Mast on Feb 03 at  7:51 pm:
> Reading it in detail I thought it allocated way too much memory and
> didn't use the full size of the allocated unsigned ints for storing
> bits.
> 
> Am I right, and is this the right way to patch code to notmuch?

Side-comments about a patch like these should go under the "---" line,
while the commit message should explain what a patch does and why.
>From your message, I didn't even understand this was fixing a bug; it
sounded like a cleanup.

  http://notmuchmail.org/contributing/#index5h2
has a bit to say about commit message contents and the notmuch git
history has a lot to say.

> 
> ---
>  lib/query.cc |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/query.cc b/lib/query.cc
> index e9c1a2d..046663a 100644
> --- a/lib/query.cc
> +++ b/lib/query.cc
> @@ -43,8 +43,8 @@ struct _notmuch_doc_id_set {
>      unsigned int bound;
>  };
>  
> -#define DOCIDSET_WORD(bit) ((bit) / sizeof (unsigned int))
> -#define DOCIDSET_BIT(bit) ((bit) % sizeof (unsigned int))
> +#define DOCIDSET_WORD(bit) ((bit) / (sizeof (unsigned int) * CHAR_BIT))
> +#define DOCIDSET_BIT(bit) ((bit) % (sizeof (unsigned int) * CHAR_BIT))

This is definitely the right thing to do, though a possibly clearer
way to fix this would be to change the type of
_notmuch_doc_id_set::bitmap to unsigned char.  Then the macros would
become simply

#define DOCIDSET_WORD(bit) ((bit) / CHAR_BIT)
#define DOCIDSET_BIT(bit) ((bit) % CHAR_BIT)

>  
>  struct visible _notmuch_threads {
>      notmuch_query_t *query;
> @@ -363,7 +363,7 @@ _notmuch_doc_id_set_init (void *ctx,
>  
>      for (unsigned int i = 0; i < arr->len; i++)
>  	max = MAX(max, g_array_index (arr, unsigned int, i));
> -    bitmap = talloc_zero_array (ctx, unsigned int, 1 + max / sizeof (*bitmap));
> +    bitmap = talloc_zero_array (ctx, unsigned int, (DOCIDSET_WORD(max) + 1) * sizeof (*bitmap));

This, however, isn't quite right.  I like your idea of using the
macros to compute the size, but the size argument should just be
  DOCIDSET_WORD(max) + 1
since talloc_zero_array expects the number of array elements, not the
number of bytes.

>  
>      if (bitmap == NULL)
>  	return FALSE;