Hey, Michal. Thanks for the review. On Thu, Aug 30 2012, Michal Sojka <sojkam1 at 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. -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 835 bytes Desc: not available URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20120830/ad38d224/attachment.pgp>