Date: Sat, 10 Dec 2011 13:24:36 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: David Bremner <david@tethera.net>
Subject: Re: [RFC PATCH] cli: factor out config handling code to get/set
 lists.
Message-ID: <20111210182436.GE2760@mit.edu>
References: <1323539438-15869-1-git-send-email-david@tethera.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1323539438-15869-1-git-send-email-david@tethera.net>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org, David Bremner <bremner@debian.org>
Precedence: list

Deduplicating this code seems like a great idea, but I don't think
macros are the way to do it; especially not one that expands to an
important top-level construct like a function definition.

What about something like

const char **
notmuch_config_get_user_other_email (notmuch_config_t *config,
                                     size_t *length)
{
    return _config_get_list (config, "user", "other_email", 
                             &config->user_other_email, length);
}

where config->user_other_email is a, say, struct _notmuch_config_list
that combines together the cached string list and length?
_config_get_list would look a lot like what you have now, but outlist
would be a struct _notmuch_config_list* instead of a const char*** and
it would always set *length (in addition to setting it in the cached
value if necessary).

Set could be similarly simple

void
notmuch_config_set_user_other_email (notmuch_config_t *config,
                                     const char *other_email[],
                                     size_t length)
{
    _config_set_list (config, "user", "other_email", &config->user_other_email,
                      other_email, length);
}

Quoth David Bremner on Dec 10 at  1:50 pm:
> From: David Bremner <bremner@debian.org>
> 
> The code is already duplicated once, and I want to add a third
> configuration item that is also a list.
> ---
> 
> Mainly I am curious if people think using macros to declare these
> "getters" and "setters" makes the code less maintainable.
> 
>  notmuch-config.c |  112 +++++++++++++++++++----------------------------------
>  1 files changed, 40 insertions(+), 72 deletions(-)
> 
> diff --git a/notmuch-config.c b/notmuch-config.c
> index 1a7ed58..33778a7 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -520,93 +520,61 @@ notmuch_config_set_user_primary_email (notmuch_config_t *config,
>      config->user_primary_email = NULL;
>  }
>  
> -const char **
> -notmuch_config_get_user_other_email (notmuch_config_t *config,
> -				     size_t *length)
> +static void
> +_config_get_list (notmuch_config_t *config,
> +		  const char *section, const char *key,
> +		  const char ***outlist, size_t *length)
>  {
> -    char **emails;
> -    size_t emails_length;
> +    char **inlist;
>      unsigned int i;
>  
> -    if (config->user_other_email == NULL) {
> -	emails = g_key_file_get_string_list (config->key_file,
> -					     "user", "other_email",
> -					     &emails_length, NULL);
> -	if (emails) {
> -	    config->user_other_email = talloc_size (config,
> -						    sizeof (char *) *
> -						    (emails_length + 1));
> -	    for (i = 0; i < emails_length; i++)
> -		config->user_other_email[i] = talloc_strdup (config->user_other_email,
> -							     emails[i]);
> -	    config->user_other_email[i] = NULL;
> -
> -	    g_strfreev (emails);
> -
> -	    config->user_other_email_length = emails_length;
> +    if (*outlist == NULL) {
> +	inlist = g_key_file_get_string_list (config->key_file,
> +					     section, key,
> +					     length, NULL);
> +	if (inlist) {
> +	    *outlist = talloc_size (config, sizeof (char *) *
> +				    (*length + 1));
> +	    for (i = 0; i < *length; i++)
> +		(*outlist)[i] = talloc_strdup (*outlist, inlist[i]);
> +	    (*outlist)[i] = NULL;
> +
> +	    g_strfreev (inlist);
> +
>  	}
>      }
>  
> -    *length = config->user_other_email_length;
> -    return config->user_other_email;
>  }
>  
> -void
> -notmuch_config_set_user_other_email (notmuch_config_t *config,
> -				     const char *other_email[],
> -				     size_t length)
> -{
> -    g_key_file_set_string_list (config->key_file,
> -				"user", "other_email",
> -				other_email, length);
> -
> -    talloc_free (config->user_other_email);
> -    config->user_other_email = NULL;
> +#define _GET_LIST(list, group, name) \
> +const char ** \
> +notmuch_config_get_##list (notmuch_config_t *config, size_t *length) \
> +{ \
> +    _config_get_list (config, group, name, &(config->list),	\
> +                      &(config->list##_length));		\
> +    *length = config->list##_length;				\
> +    return config->list; \
>  }
>  
> -const char **
> -notmuch_config_get_new_tags (notmuch_config_t *config,
> -			     size_t *length)
> -{
> -    char **tags;
> -    size_t tags_length;
> -    unsigned int i;
> +_GET_LIST (user_other_email, "user", "other_email");
> +_GET_LIST (new_tags, "new", "tags");
>  
> -    if (config->new_tags == NULL) {
> -	tags = g_key_file_get_string_list (config->key_file,
> -					   "new", "tags",
> -					   &tags_length, NULL);
> -	if (tags) {
> -	    config->new_tags = talloc_size (config,
> -					    sizeof (char *) *
> -					    (tags_length + 1));
> -	    for (i = 0; i < tags_length; i++)
> -		config->new_tags[i] = talloc_strdup (config->new_tags,
> -						     tags[i]);
> -	    config->new_tags[i] = NULL;
> -
> -	    g_strfreev (tags);
> -
> -	    config->new_tags_length = tags_length;
> -	}
> -    }
> +#undef _GET_LIST
>  
> -    *length = config->new_tags_length;
> -    return config->new_tags;
> +#define _SET_LIST(list, group, name) \
> +void \
> +notmuch_config_set_##list (notmuch_config_t *config, const char *list[], \
> +			   size_t length) \
> +{ \
> +    g_key_file_set_string_list (config->key_file, group, name, list, length); \
> +    talloc_free (config->list);                                               \
> +    config->list = NULL;                                                      \
>  }
>  
> -void
> -notmuch_config_set_new_tags (notmuch_config_t *config,
> -			     const char *new_tags[],
> -			     size_t length)
> -{
> -    g_key_file_set_string_list (config->key_file,
> -				"new", "tags",
> -				new_tags, length);
> +_SET_LIST(user_other_email, "user", "other_email");
> +_SET_LIST(new_tags, "new", "tags");
>  
> -    talloc_free (config->new_tags);
> -    config->new_tags = NULL;
> -}
> +#undef _SET_LIST
>  
>  /* Given a configuration item of the form <group>.<key> return the
>   * component group and key. If any error occurs, print a message on