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;