From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 19683431FB6 for ; Sun, 3 Feb 2013 12:01:34 -0800 (PST) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: -0.7 X-Spam-Level: X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled Received: from olra.theworths.org ([127.0.0.1]) by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 3Bx+QfTL8Dd3 for ; Sun, 3 Feb 2013 12:01:32 -0800 (PST) Received: from dmz-mailsec-scanner-3.mit.edu (DMZ-MAILSEC-SCANNER-3.MIT.EDU [18.9.25.14]) by olra.theworths.org (Postfix) with ESMTP id 17843431FAE for ; Sun, 3 Feb 2013 12:01:32 -0800 (PST) X-AuditID: 1209190e-b7f266d0000008cb-e9-510ec21b3dbc Received: from mailhub-auth-4.mit.edu ( [18.7.62.39]) by dmz-mailsec-scanner-3.mit.edu (Symantec Messaging Gateway) with SMTP id 99.3E.02251.B12CE015; Sun, 3 Feb 2013 15:01:31 -0500 (EST) Received: from outgoing.mit.edu (OUTGOING-AUTH-1.MIT.EDU [18.9.28.11]) by mailhub-auth-4.mit.edu (8.13.8/8.9.2) with ESMTP id r13K1UlO014252; Sun, 3 Feb 2013 15:01:30 -0500 Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91]) (authenticated bits=0) (User authenticated as amdragon@ATHENA.MIT.EDU) by outgoing.mit.edu (8.13.8/8.12.4) with ESMTP id r13K1R7f024955 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT); Sun, 3 Feb 2013 15:01:29 -0500 Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.80) (envelope-from ) id 1U25kl-0004uM-Hj; Sun, 03 Feb 2013 15:01:27 -0500 Date: Sun, 3 Feb 2013 15:01:27 -0500 From: Austin Clements To: Robert Mast 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) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprAKsWRmVeSWpSXmKPExsUixG6nrit9iC/Q4PVxEYuTHe4W12/OZHZg 8ni26hazx5mDzxkDmKK4bFJSczLLUov07RK4Mvb/38pc8FyoYsnd9SwNjG/4uhg5OSQETCR2 /W9hh7DFJC7cW8/WxcjFISSwj1Fi1vwrrBDOBkaJg/2HmSCcU0wS6z+ehCpbwijxuvM4C0g/ i4CKxJ5Xu8FsNgENiW37lzOC2CIC+hJbT3xlBrGZBaQlvv1uBpskLNDPKHGv8wwTSIJXQFvi x+NnYEVCAr4S2x+3s0HEBSVOznzCAtGsJXHj30ugeg6wQcv/cYCEOQX8JB4fecYKYosC3TDl 5Da2CYxCs5B0z0LSPQuhewEj8ypG2ZTcKt3cxMyc4tRk3eLkxLy81CJdY73czBK91JTSTYyg wOaU5NvB+PWg0iFGAQ5GJR7eCdt4A4VYE8uKK3MPMUpyMCmJ8noc4AsU4kvKT6nMSCzOiC8q zUktPsQowcGsJMJ71Qgox5uSWFmVWpQPk5LmYFES572SctNfSCA9sSQ1OzW1ILUIJivDwaEk wct9EKhRsCg1PbUiLTOnBCHNxMEJMpwHaLg8SA1vcUFibnFmOkT+FKMux64JHc8ZhVjy8vNS pcR5v4BcJwBSlFGaBzcHlpBeMYoDvSXM+xKkigeYzOAmvQJawgS0pK2SG2RJSSJCSqqBMSNl j6Dh84eHFacyMbLf4Xz1X+/Ulx6nhSGfptY+1fO9sMPy/HrznbkPvwQFNdkK8NjNPrOQheV9 V4hrbHRZBm+7MYtJxbHgaw93zP+eGVe0ITNdf2625bHJPMvtDkU/W3hEINRXs9LAaZ78w8kz N/wzuHratWmvrv6SaveqxjV+jnFLrzRLKLEUZyQaajEXFScCABCaC8sjAwAA Cc: notmuch@notmuchmail.org X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 03 Feb 2013 20:01:34 -0000 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;