From: Austin Clements Date: Sun, 3 Feb 2013 20:01:27 +0000 (+1900) Subject: Re: [PATCH] This patch is a little finger excercise for working with git. I found... X-Git-Url: http://git.tremily.us/gitweb.cgi?a=commitdiff_plain;h=853570b45ae12936acfaf1caf04c065ba3092989;p=notmuch-archives.git 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. --- diff --git a/78/5c3917617482b3e376918327740e90afc47827 b/78/5c3917617482b3e376918327740e90afc47827 new file mode 100644 index 000000000..786b88fea --- /dev/null +++ b/78/5c3917617482b3e376918327740e90afc47827 @@ -0,0 +1,141 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +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;