On Sun, 30 Aug 2015, David Bremner <da...@tethera.net> wrote:
> Jani Nikula <j...@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.
_______________________________________________
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
