Hey, Michal. Thanks for the review. On Thu, Aug 30 2012, Michal Sojka <[email protected]> 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.
pgphr7xfvRHdR.pgp
Description: PGP signature
_______________________________________________ notmuch mailing list [email protected] http://notmuchmail.org/mailman/listinfo/notmuch
