Re: [PATCH 01/11] lib: new thread addresses structure

[Jameson Graef Rollins <jrollins@finestructure.net>]

[-- Attachment #1: Type: text/plain, Size: 2689 bytes --]

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.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]