[PATCH 01/11] lib: new thread addresses structure
Austin Clements
amdragon at MIT.EDU
Sat Sep 8 10:24:03 PDT 2012
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
More information about the notmuch
mailing list