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

[Michal Sojka <sojkam1@fel.cvut.cz>]

Hi Jameson,

some comments below.

On Mon, Aug 20 2012, Jameson Graef Rollins wrote:
> This new structure holds addresses associated with a thread, both
> matched and unmatched.  Initially this will be used to replace the
> existing infrastructure for storing the addresses of thread authors.
> Further patches will use it to store the addresses of threads
> recipients.
>
> Init and destructor functions are included, as well as a function to
> add addresses to a struct, either "matched" or not.
> ---
>  lib/notmuch.h |    1 +
>  lib/thread.cc |  116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 117 insertions(+)
>
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index 3633bed..6acd38d 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -118,6 +118,7 @@ typedef struct _notmuch_database notmuch_database_t;
>  typedef struct _notmuch_query notmuch_query_t;
>  typedef struct _notmuch_threads notmuch_threads_t;
>  typedef struct _notmuch_thread notmuch_thread_t;
> +typedef struct _notmuch_thread_addresses notmuch_thread_addresses_t;
>  typedef struct _notmuch_messages notmuch_messages_t;
>  typedef struct _notmuch_message notmuch_message_t;
>  typedef struct _notmuch_tags notmuch_tags_t;
> diff --git a/lib/thread.cc b/lib/thread.cc
> index e976d64..7af9eeb 100644
> --- a/lib/thread.cc
> +++ b/lib/thread.cc
> @@ -24,6 +24,14 @@
>  #include <gmime/gmime.h>
>  #include <glib.h> /* GHashTable */
>  
> +struct visible _notmuch_thread_addresses {
> +    GHashTable *matched_hash;
> +    GPtrArray *matched_array;
> +    GHashTable *unmatched_hash;
> +    GPtrArray *unmatched_array;
> +    char *string;
> +};
> +
>  struct visible _notmuch_thread {
>      notmuch_database_t *notmuch;
>      char *thread_id;
> @@ -44,6 +52,18 @@ struct visible _notmuch_thread {
>  };
>  
>  static int
> +_notmuch_thread_addresses_destructor (notmuch_thread_addresses_t *addresses)
> +{
> +    g_hash_table_unref (addresses->matched_hash);
> +    g_hash_table_unref (addresses->unmatched_hash);
> +    g_ptr_array_free (addresses->matched_array, TRUE);
> +    g_ptr_array_free (addresses->unmatched_array, TRUE);
> +    addresses->matched_array = NULL;
> +    addresses->unmatched_array = NULL;
> +    return 0;
> +}
> +
> +static int
>  _notmuch_thread_destructor (notmuch_thread_t *thread)
>  {
>      g_hash_table_unref (thread->authors_hash);
> @@ -64,6 +84,81 @@ _notmuch_thread_destructor (notmuch_thread_t *thread)
>      return 0;
>  }
>  
> +/* Add address to a thread addresses struct.  If matched is TRUE, then
> + * the address will be added to the matched list.*/
> +static void
> +_thread_add_address (notmuch_thread_addresses_t *addresses,
> +		     const char *address,
> +		     notmuch_bool_t matched)
> +{
> +    char *address_copy;
> +    GHashTable *hash;
> +    GPtrArray *array;
> +
> +    if (matched) {
> +	hash = addresses->matched_hash;
> +	array = addresses->matched_array;
> +    } else {
> +	hash = addresses->unmatched_hash;
> +	array = addresses->unmatched_array;
> +    }
> +
> +    if (address == NULL)
> +	return;
> +
> +    if (g_hash_table_lookup_extended (hash, address, NULL, NULL))
> +	return;
> +
> +    address_copy = talloc_strdup (addresses, address);
> +
> +    g_hash_table_insert (hash, address_copy, NULL);
> +
> +    g_ptr_array_add (array, address_copy);
> +}
> +
> +/* 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.

> +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.

-Michal

> +
> +	first_non_matched_address = 0;
> +    }
> +}
> +
>  /* Add each author of the thread to the thread's authors_hash and to
>   * the thread's authors_array. */
>  static void
> @@ -382,6 +477,27 @@ _resolve_thread_relationships (unused (notmuch_thread_t *thread))
>       */
>  }
>  
> +/* Initialize a thread addresses struct. */
> +notmuch_thread_addresses_t *
> +_thread_addresses_init (const void *ctx)
> +{
> +    notmuch_thread_addresses_t *addresses;
> +
> +    addresses = talloc (ctx, notmuch_thread_addresses_t);
> +    if (unlikely (addresses == NULL))
> +	return NULL;
> +
> +    addresses->matched_hash = g_hash_table_new_full (g_str_hash, g_str_equal,
> +						     NULL, NULL);
> +    addresses->matched_array = g_ptr_array_new ();
> +    addresses->unmatched_hash = g_hash_table_new_full (g_str_hash, g_str_equal,
> +						       NULL, NULL);
> +    addresses->unmatched_array = g_ptr_array_new ();
> +    addresses->string = NULL;
> +
> +    return addresses;
> +}
> +
>  /* Create a new notmuch_thread_t object by finding the thread
>   * containing the message with the given doc ID, treating any messages
>   * contained in match_set as "matched".  Remove all messages in the
> -- 
> 1.7.10.4