Date: Sat, 8 Sep 2012 13:24:03 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Jameson Graef Rollins <jrollins@finestructure.net>
Subject: Re: [PATCH 01/11] lib: new thread addresses structure
Message-ID: <20120908172403.GA21371@mit.edu>
References: <1345427570-26518-1-git-send-email-jrollins@finestructure.net>
	<1345427570-26518-2-git-send-email-jrollins@finestructure.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1345427570-26518-2-git-send-email-jrollins@finestructure.net>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: Notmuch Mail <notmuch@notmuchmail.org>
Precedence: list

Quoth Jameson Graef Rollins on Aug 19 at  6:52 pm:
> 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);

The second argument should be FALSE for both of these, since the
pointers contained in these pointer arrays are talloc-managed.

> +    addresses->matched_array = NULL;
> +    addresses->unmatched_array = NULL;

Is this necessary?  (Obviously there's no harm.)

> +    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,

_thread_addresses_add?

> +		     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. */
> +static void
> +_resolve_thread_addresses_string (notmuch_thread_addresses_t *addresses)

A better API for this would be to return a const char*.  On the first
call (when addresses->string is NULL), construct the string and return
it.  On subsequent calls, just immediately return addresses->string.
The approach you're using here makes sense from an incremental
perspective, but since you're introducing an abstraction for thread
address lists, I think it makes more sense to look at it from an API
perspective.

Also, the name should probably start with thread_addresses, since
that's the abstraction it's part of.
_thread_addresses_resolve_string?  _thread_addresses_to_string?

> +{
> +    unsigned int i;
> +    char *address;
> +    int first_non_matched_address = 1;
> +
> +    /* First, list all matched addressses in date order. */

Michal's comment about this not necessarily being date order applies
here, too.

> +    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);
> +	}

I second Michal's comments on this code; especially the one about
using talloc_asprintf_append, even if you don't combine the two
branches.  Currently this code leaks O(n^2) memory (in a talloc
context, so it's not permanent, but it's still unfortunate).

> +
> +	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)

This should be "create" instead of "init".  Also, this should probably
be static, given that everything else is.  If there's reason to make
it non-static, then it should also start with _notmuch and have a
prototype in notmuch-private.h.

> +{
> +    notmuch_thread_addresses_t *addresses;
> +
> +    addresses = talloc (ctx, notmuch_thread_addresses_t);
> +    if (unlikely (addresses == NULL))
> +	return NULL;

You should set _notmuch_thread_addresses_destructor as the talloc
destructor here, rather than calling it by hand from
_notmuch_thread_destructor (in patch 2).

> +
> +    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