From: Jameson Graef Rollins <jrollins@finestructure.net>
To: Michal Sojka <sojkam1@fel.cvut.cz>, Notmuch Mail <notmuch@notmuchmail.org>
Subject: Re: [PATCH 01/11] lib: new thread addresses structure
In-Reply-To: <87y5kw4cfo.fsf@steelpick.2x.cz>
References: <1345427570-26518-1-git-send-email-jrollins@finestructure.net>
	<1345427570-26518-2-git-send-email-jrollins@finestructure.net>
	<87y5kw4cfo.fsf@steelpick.2x.cz>
User-Agent: Notmuch/0.14+6~gb305ce9 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Thu, 30 Aug 2012 09:33:56 -0700
Message-ID: <874nnkpcej.fsf@servo.finestructure.net>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha256; protocol="application/pgp-signature"
Precedence: list

--=-=-=

Hey, Michal.  Thanks for the review.

On Thu, Aug 30 2012, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
>> +/* Construct an addresses string from matched and unmatched addresses
>> + * in notmuch_thread_addresses_t. The string contains matched
>> + * addresses first, then non-matched addresses (with the two groups
>> + * separated by '|'). Within each group, addresses are listed in date
>> + * order. */
>
> I'd say the the addresses are listed in the order in which they have
> been added, which might or might not be the date order.

I actually did not write the _resolve_thread_addresses_string function.
I left the logic and behavior unchanged, and just modified to use the
new addresses structure.  If we think the logic should be changed, maybe
we should do that in a separate patch.

>> +static void
>> +_resolve_thread_addresses_string (notmuch_thread_addresses_t *addresses)
>> +{
>> +    unsigned int i;
>> +    char *address;
>> +    int first_non_matched_address = 1;
>> +
>> +    /* First, list all matched addressses in date order. */
>> +    for (i = 0; i < addresses->matched_array->len; i++) {
>> +	address = (char *) g_ptr_array_index (addresses->matched_array, i);
>> +	if (addresses->string)
>> +	    addresses->string = talloc_asprintf (addresses, "%s, %s",
>> +						 addresses->string,
>> +						 address);
>> +	else
>> +	    addresses->string = address;
>> +    }
>> +
>> +    /* Next, append any non-matched addresses that haven't already appeared. */
>> +    for (i = 0; i < addresses->unmatched_array->len; i++) {
>> +	address = (char *) g_ptr_array_index (addresses->unmatched_array, i);
>> +	if (g_hash_table_lookup_extended (addresses->matched_hash,
>> +					  address, NULL, NULL))
>> +	    continue;
>> +	if (first_non_matched_address) {
>> +	    addresses->string = talloc_asprintf (addresses, "%s| %s",
>> +						 addresses->string,
>> +						 address);
>> +	} else {
>> +	    addresses->string = talloc_asprintf (addresses, "%s, %s",
>> +						 addresses->string,
>> +						 address);
>> +	}
>
> Simpler would be:
>
> addresses->string = talloc_asprintf (addresses, "%s%c %s",
>                                      addresses->string,
>                                      first_non_matched_address ? '|' : ','
>                                      address);
>
> Also, you might want to talloc_free the old address->string to not waste
> memory in the case of long lived notmuch_thread_addresses_t object. Or
> better use talloc_asprintf_append() function, which hopefully implements
> freeing internally.

This looks like a nice simplification, but see comment above.

I'll look into including these changes in the next version of the
series.

jamie.

--=-=-=
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)

iQIcBAEBCAAGBQJQP5X0AAoJEO00zqvie6q8+YQQAKrKycrpRrNJdqWg7d/0SwSP
BU0GZsEPB2p7i8uKpwQ6CdnOKXHxme9mnMlD6MpTR5SZqqoOZaEkWRK/E5e8FBh4
8Zh1OMgpwrPIy/hQAwMJ7/z718P4h34wjA8utB9WtHUJH6f+Hm2IDfV7MeHk/4mz
FJmv87JH4cmj2Q8Clgp8HTbLeg15wyatc9eV+AT2AeRVcSo56AHBXTj34Q6Uj/3M
Usq/7jLn7kukEr+pMcW+sXJWSEdV9asbAY7a7O63cuFg9mHJamkZAtajvJ5cBIw6
y8D/SxWzv6XCg20oSgnhxfXxoE6WWOgdeDMQLdoT9KP/ywcvU4X9H7/bOkxnsp5G
+n+BKgDtbBE+CFVgYZ2MMZVWVkMh57xilvfsQXiRpj8UQvrgDxNnpKMxbD7MJln4
72xLN1JbkdBjX0iGxdKfLs8o4DqLgbVFC4x6WJQCqmViY3i26oK8MVt6kwsTQFuY
heoqqj5SsCSQfCdnmS6UiVnzhIw4kJK5+U7upHt/gL+JzDQ3kRcXQ740upUK2PCA
hKtmquLgLxx6Tk2ksgYuT5TPBe92AcdBPirv/wSmM0Py92S02IWl26Rtjqm5GEVw
iqSpLb1gvj2Xzj2QxEFMA6TX5voUUw/9tzrBb+IeatRxd6jMh7i2FF1oQOEQAsgr
6swbWsOcduFoJRmv6nm0
=k1/c
-----END PGP SIGNATURE-----
--=-=-=--