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 <brem...@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 _______________________________________________ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch