On Sun, 30 Aug 2015, David Bremner <david@tethera.net> wrote: > Jani Nikula <jani@nikula.org> writes: > >> >> +static int >> +strcase_equal (const void *a, const void *b) >> +{ >> + return strcasecmp (a, b) == 0; >> +} >> + >> +static unsigned int >> +strcase_hash (const void *ptr) >> +{ >> + const char *s = ptr; >> + >> + /* This is the djb2 hash. */ >> + unsigned int hash = 5381; >> + while (s && *s) { >> + hash = ((hash << 5) + hash) + tolower (*s); >> + s++; >> + } >> + >> + return hash; >> +} >> + > > as discussed, these functions probably need to be factored out into > libutil. Ack. > >> + l = g_list_find_custom (list, mailbox, mailbox_compare); >> + if (l) { >> + talloc_free (mailbox); >> + mailbox = l->data; >> + mailbox->count++; >> + return TRUE; >> + } > > I found this use of mailbox as a temporary variable confusing; despite > the obvious return I thought it might have something to do with the > g_list_append below. Maybe just make a block scope temporary variable? This is how the function would turn out with that. Better, I guess? I also tried to think of ways to combine the two g_list_append paths here, but in the end doing it like this has most clarity I think. static notmuch_bool_t is_duplicate (const search_context_t *ctx, const char *name, const char *addr) { char *key; GList *list, *l; mailbox_t *mailbox; list = g_hash_table_lookup (ctx->addresses, addr); if (list) { mailbox_t find = { .name = name, .addr = addr, }; l = g_list_find_custom (list, &find, mailbox_compare); if (l) { mailbox = l->data; mailbox->count++; return TRUE; } mailbox = new_mailbox (ctx->format, name, addr); if (! mailbox) return FALSE; g_list_append (list, mailbox); return FALSE; } key = talloc_strdup (ctx->format, addr); if (! key) return FALSE; mailbox = new_mailbox (ctx->format, name, addr); if (! mailbox) return FALSE; list = g_list_append (NULL, mailbox); if (! list) return FALSE; g_hash_table_insert (ctx->addresses, key, list); return FALSE; } >> + >> + g_list_append (list, mailbox); >> + return FALSE; >> } >> >> - mailbox = new_mailbox (ctx->format, name, addr); >> - if (! mailbox) >> + key = talloc_strdup (ctx->format, addr); >> + if (! key) >> return FALSE; > > I guess this doesn't make the error handling worse; both old and new > code silently ignore OOM if I understand correctly. Do you happen to > understand the original choice of using ctx->format rather than that > ctx->notmuch for a talloc parent? it doesn't seem to get deallocated any > earlier. I don't know or understand that part of the history. It doesn't really matter though because the deallocation is explicitly done on all keys and values via g_hash_table_unref. BR, Jani.