[PATCH 1/2] cli: abstract common config get/set code

[PATCH 1/2] cli: abstract common config get/set code

[Jani Nikula]

Pretty straightforward abstraction similar to get/set list.
---
 notmuch-config.c |   80 ++++++++++++++++++++----------------------------------
 1 file changed, 29 insertions(+), 51 deletions(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index befe9b5..305d213 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -496,6 +496,29 @@ notmuch_config_is_new (notmuch_config_t *config)
     return config->is_new;
 }
 
+static const char *
+_config_get (notmuch_config_t *config, char **field,
+	     const char *group, const char *key)
+{
+    if (*field == NULL) {
+	char *value;
+	value = g_key_file_get_string (config->key_file, group, key, NULL);
+	if (value) {
+	    *field = talloc_strdup (config, value);
+	    free (value);
+	}
+    }
+    return *field;
+}
+
+static void
+_config_set (notmuch_config_t *config, char **field,
+	     const char *group, const char *key, const char *value)
+{
+    g_key_file_set_string (config->key_file, group, key, value);
+    talloc_free (*field);
+    *field = NULL;
+}
 
 static const char **
 _config_get_list (notmuch_config_t *config,
@@ -542,85 +565,40 @@ _config_set_list (notmuch_config_t *config,
 const char *
 notmuch_config_get_database_path (notmuch_config_t *config)
 {
-    char *path;
-
-    if (config->database_path == NULL) {
-	path = g_key_file_get_string (config->key_file,
-				      "database", "path", NULL);
-	if (path) {
-	    config->database_path = talloc_strdup (config, path);
-	    free (path);
-	}
-    }
-
-    return config->database_path;
+    return _config_get (config, &config->database_path, "database", "path");
 }
 
 void
 notmuch_config_set_database_path (notmuch_config_t *config,
 				  const char *database_path)
 {
-    g_key_file_set_string (config->key_file,
-			   "database", "path", database_path);
-
-    talloc_free (config->database_path);
-    config->database_path = NULL;
+    _config_set (config, &config->database_path, "database", "path", database_path);
 }
 
 const char *
 notmuch_config_get_user_name (notmuch_config_t *config)
 {
-    char *name;
-
-    if (config->user_name == NULL) {
-	name = g_key_file_get_string (config->key_file,
-				      "user", "name", NULL);
-	if (name) {
-	    config->user_name = talloc_strdup (config, name);
-	    free (name);
-	}
-    }
-
-    return config->user_name;
+    return _config_get (config, &config->user_name, "user", "name");
 }
 
 void
 notmuch_config_set_user_name (notmuch_config_t *config,
 			      const char *user_name)
 {
-    g_key_file_set_string (config->key_file,
-			   "user", "name", user_name);
-
-    talloc_free (config->user_name);
-    config->user_name = NULL;
+    _config_set (config, &config->user_name, "user", "name", user_name);
 }
 
 const char *
 notmuch_config_get_user_primary_email (notmuch_config_t *config)
 {
-    char *email;
-
-    if (config->user_primary_email == NULL) {
-	email = g_key_file_get_string (config->key_file,
-				       "user", "primary_email", NULL);
-	if (email) {
-	    config->user_primary_email = talloc_strdup (config, email);
-	    free (email);
-	}
-    }
-
-    return config->user_primary_email;
+    return _config_get (config, &config->user_primary_email, "user", "primary_email");
 }
 
 void
 notmuch_config_set_user_primary_email (notmuch_config_t *config,
 				       const char *primary_email)
 {
-    g_key_file_set_string (config->key_file,
-			   "user", "primary_email", primary_email);
-
-    talloc_free (config->user_primary_email);
-    config->user_primary_email = NULL;
+    _config_set (config, &config->user_primary_email, "user", "primary_email", primary_email);
 }
 
 const char **
-- 
1.7.10.4

[PATCH 2/2] cli: define config getters and setters using a macro

[Jani Nikula]

There's plenty of duplicated code in defining the functions for config
get/set. Add macros to define the functions.

---

This might be a bit too tricky for some people's tastes... let's see!
;)
---
 notmuch-config.c |  141 ++++++++++++++----------------------------------------
 1 file changed, 37 insertions(+), 104 deletions(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index 305d213..fcee0fc 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -520,6 +520,18 @@ _config_set (notmuch_config_t *config, char **field,
     *field = NULL;
 }
 
+#define DEFINE_CONFIG_GET(group, key) \
+    const char *							\
+    notmuch_config_get_ ## group ## _ ## key (notmuch_config_t *config) { \
+	return _config_get (config, &config->group ## _ ## key, #group, #key); \
+    }
+
+#define DEFINE_CONFIG_SET(group, key)				\
+    void								\
+    notmuch_config_set_ ## group ## _ ## key (notmuch_config_t *config, const char *value) { \
+	_config_set (config, &config->group ## _ ## key, #group, #key, value); \
+    }
+
 static const char **
 _config_get_list (notmuch_config_t *config,
 		  const char *section, const char *key,
@@ -562,112 +574,33 @@ _config_set_list (notmuch_config_t *config,
     *config_var = NULL;
 }
 
-const char *
-notmuch_config_get_database_path (notmuch_config_t *config)
-{
-    return _config_get (config, &config->database_path, "database", "path");
-}
-
-void
-notmuch_config_set_database_path (notmuch_config_t *config,
-				  const char *database_path)
-{
-    _config_set (config, &config->database_path, "database", "path", database_path);
-}
-
-const char *
-notmuch_config_get_user_name (notmuch_config_t *config)
-{
-    return _config_get (config, &config->user_name, "user", "name");
-}
-
-void
-notmuch_config_set_user_name (notmuch_config_t *config,
-			      const char *user_name)
-{
-    _config_set (config, &config->user_name, "user", "name", user_name);
-}
-
-const char *
-notmuch_config_get_user_primary_email (notmuch_config_t *config)
-{
-    return _config_get (config, &config->user_primary_email, "user", "primary_email");
-}
-
-void
-notmuch_config_set_user_primary_email (notmuch_config_t *config,
-				       const char *primary_email)
-{
-    _config_set (config, &config->user_primary_email, "user", "primary_email", primary_email);
-}
-
-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),
-			     &(config->user_other_email_length), length);
-}
-
-const char **
-notmuch_config_get_new_tags (notmuch_config_t *config,   size_t *length)
-{
-    return _config_get_list (config, "new", "tags",
-			     &(config->new_tags),
-			     &(config->new_tags_length), length);
-}
-
-const char **
-notmuch_config_get_new_ignore (notmuch_config_t *config, size_t *length)
-{
-    return _config_get_list (config, "new", "ignore",
-			     &(config->new_ignore),
-			     &(config->new_ignore_length), length);
-}
-
-void
-notmuch_config_set_user_other_email (notmuch_config_t *config,
-				     const char *list[],
-				     size_t length)
-{
-    _config_set_list (config, "user", "other_email", list, length,
-		     &(config->user_other_email));
-}
-
-void
-notmuch_config_set_new_tags (notmuch_config_t *config,
-				     const char *list[],
-				     size_t length)
-{
-    _config_set_list (config, "new", "tags", list, length,
-		     &(config->new_tags));
-}
-
-void
-notmuch_config_set_new_ignore (notmuch_config_t *config,
-			       const char *list[],
-			       size_t length)
-{
-    _config_set_list (config, "new", "ignore", list, length,
-		     &(config->new_ignore));
-}
+#define DEFINE_CONFIG_GET_LIST(group, key)				\
+    const char **							\
+    notmuch_config_get_ ## group ## _ ## key (notmuch_config_t *config, size_t *length) { \
+	return _config_get_list (config, #group, #key, &config->group ## _ ## key, &config->group ## _ ## key ## _ ## length, length); \
+    }
 
-const char **
-notmuch_config_get_search_exclude_tags (notmuch_config_t *config, size_t *length)
-{
-    return _config_get_list (config, "search", "exclude_tags",
-			     &(config->search_exclude_tags),
-			     &(config->search_exclude_tags_length), length);
-}
+#define DEFINE_CONFIG_SET_LIST(group, key)				\
+    void								\
+    notmuch_config_set_ ## group ## _ ## key (notmuch_config_t *config, const char *list[], size_t length) { \
+	_config_set_list (config, #group, #key, list, length, &config->group ## _ ## key); \
+    }
 
-void
-notmuch_config_set_search_exclude_tags (notmuch_config_t *config,
-				      const char *list[],
-				      size_t length)
-{
-    _config_set_list (config, "search", "exclude_tags", list, length,
-		      &(config->search_exclude_tags));
-}
+DEFINE_CONFIG_GET(database, path);
+DEFINE_CONFIG_SET(database, path);
+DEFINE_CONFIG_GET(user, name);
+DEFINE_CONFIG_SET(user, name);
+DEFINE_CONFIG_GET(user, primary_email);
+DEFINE_CONFIG_SET(user, primary_email);
+
+DEFINE_CONFIG_GET_LIST(user, other_email);
+DEFINE_CONFIG_SET_LIST(user, other_email);
+DEFINE_CONFIG_GET_LIST(new, tags);
+DEFINE_CONFIG_SET_LIST(new, tags);
+DEFINE_CONFIG_GET_LIST(new, ignore);
+DEFINE_CONFIG_SET_LIST(new, ignore);
+DEFINE_CONFIG_GET_LIST(search, exclude_tags);
+DEFINE_CONFIG_SET_LIST(search, exclude_tags);
 
 /* Given a configuration item of the form <group>.<key> return the
  * component group and key. If any error occurs, print a message on
-- 
1.7.10.4

Re: [PATCH 2/2] cli: define config getters and setters using a macro

[Tomi Ollila]

On Sun, Aug 18 2013, Jani Nikula <jani@nikula.org> wrote:

> There's plenty of duplicated code in defining the functions for config
> get/set. Add macros to define the functions.
>
> ---
>
> This might be a bit too tricky for some people's tastes... let's see!
> ;)

Yes, a bit tricky. Nevertheless I like these...

Additionally id:1376839205-5115-1-git-send-email-jani@nikula.org LGTM.

Tomi


> ---
>  notmuch-config.c |  141 ++++++++++++++----------------------------------------
>  1 file changed, 37 insertions(+), 104 deletions(-)
>
> diff --git a/notmuch-config.c b/notmuch-config.c
> index 305d213..fcee0fc 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -520,6 +520,18 @@ _config_set (notmuch_config_t *config, char **field,
>      *field = NULL;
>  }
>  
> +#define DEFINE_CONFIG_GET(group, key) \
> +    const char *							\
> +    notmuch_config_get_ ## group ## _ ## key (notmuch_config_t *config) { \
> +	return _config_get (config, &config->group ## _ ## key, #group, #key); \
> +    }
> +
> +#define DEFINE_CONFIG_SET(group, key)				\
> +    void								\
> +    notmuch_config_set_ ## group ## _ ## key (notmuch_config_t *config, const char *value) { \
> +	_config_set (config, &config->group ## _ ## key, #group, #key, value); \
> +    }
> +
>  static const char **
>  _config_get_list (notmuch_config_t *config,
>  		  const char *section, const char *key,
> @@ -562,112 +574,33 @@ _config_set_list (notmuch_config_t *config,
>      *config_var = NULL;
>  }
>  
> -const char *
> -notmuch_config_get_database_path (notmuch_config_t *config)
> -{
> -    return _config_get (config, &config->database_path, "database", "path");
> -}
> -
> -void
> -notmuch_config_set_database_path (notmuch_config_t *config,
> -				  const char *database_path)
> -{
> -    _config_set (config, &config->database_path, "database", "path", database_path);
> -}
> -
> -const char *
> -notmuch_config_get_user_name (notmuch_config_t *config)
> -{
> -    return _config_get (config, &config->user_name, "user", "name");
> -}
> -
> -void
> -notmuch_config_set_user_name (notmuch_config_t *config,
> -			      const char *user_name)
> -{
> -    _config_set (config, &config->user_name, "user", "name", user_name);
> -}
> -
> -const char *
> -notmuch_config_get_user_primary_email (notmuch_config_t *config)
> -{
> -    return _config_get (config, &config->user_primary_email, "user", "primary_email");
> -}
> -
> -void
> -notmuch_config_set_user_primary_email (notmuch_config_t *config,
> -				       const char *primary_email)
> -{
> -    _config_set (config, &config->user_primary_email, "user", "primary_email", primary_email);
> -}
> -
> -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),
> -			     &(config->user_other_email_length), length);
> -}
> -
> -const char **
> -notmuch_config_get_new_tags (notmuch_config_t *config,   size_t *length)
> -{
> -    return _config_get_list (config, "new", "tags",
> -			     &(config->new_tags),
> -			     &(config->new_tags_length), length);
> -}
> -
> -const char **
> -notmuch_config_get_new_ignore (notmuch_config_t *config, size_t *length)
> -{
> -    return _config_get_list (config, "new", "ignore",
> -			     &(config->new_ignore),
> -			     &(config->new_ignore_length), length);
> -}
> -
> -void
> -notmuch_config_set_user_other_email (notmuch_config_t *config,
> -				     const char *list[],
> -				     size_t length)
> -{
> -    _config_set_list (config, "user", "other_email", list, length,
> -		     &(config->user_other_email));
> -}
> -
> -void
> -notmuch_config_set_new_tags (notmuch_config_t *config,
> -				     const char *list[],
> -				     size_t length)
> -{
> -    _config_set_list (config, "new", "tags", list, length,
> -		     &(config->new_tags));
> -}
> -
> -void
> -notmuch_config_set_new_ignore (notmuch_config_t *config,
> -			       const char *list[],
> -			       size_t length)
> -{
> -    _config_set_list (config, "new", "ignore", list, length,
> -		     &(config->new_ignore));
> -}
> +#define DEFINE_CONFIG_GET_LIST(group, key)				\
> +    const char **							\
> +    notmuch_config_get_ ## group ## _ ## key (notmuch_config_t *config, size_t *length) { \
> +	return _config_get_list (config, #group, #key, &config->group ## _ ## key, &config->group ## _ ## key ## _ ## length, length); \
> +    }
>  
> -const char **
> -notmuch_config_get_search_exclude_tags (notmuch_config_t *config, size_t *length)
> -{
> -    return _config_get_list (config, "search", "exclude_tags",
> -			     &(config->search_exclude_tags),
> -			     &(config->search_exclude_tags_length), length);
> -}
> +#define DEFINE_CONFIG_SET_LIST(group, key)				\
> +    void								\
> +    notmuch_config_set_ ## group ## _ ## key (notmuch_config_t *config, const char *list[], size_t length) { \
> +	_config_set_list (config, #group, #key, list, length, &config->group ## _ ## key); \
> +    }
>  
> -void
> -notmuch_config_set_search_exclude_tags (notmuch_config_t *config,
> -				      const char *list[],
> -				      size_t length)
> -{
> -    _config_set_list (config, "search", "exclude_tags", list, length,
> -		      &(config->search_exclude_tags));
> -}
> +DEFINE_CONFIG_GET(database, path);
> +DEFINE_CONFIG_SET(database, path);
> +DEFINE_CONFIG_GET(user, name);
> +DEFINE_CONFIG_SET(user, name);
> +DEFINE_CONFIG_GET(user, primary_email);
> +DEFINE_CONFIG_SET(user, primary_email);
> +
> +DEFINE_CONFIG_GET_LIST(user, other_email);
> +DEFINE_CONFIG_SET_LIST(user, other_email);
> +DEFINE_CONFIG_GET_LIST(new, tags);
> +DEFINE_CONFIG_SET_LIST(new, tags);
> +DEFINE_CONFIG_GET_LIST(new, ignore);
> +DEFINE_CONFIG_SET_LIST(new, ignore);
> +DEFINE_CONFIG_GET_LIST(search, exclude_tags);
> +DEFINE_CONFIG_SET_LIST(search, exclude_tags);
>  
>  /* Given a configuration item of the form <group>.<key> return the
>   * component group and key. If any error occurs, print a message on
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/2] cli: abstract common config get/set code

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> Pretty straightforward abstraction similar to get/set list.
> ---
>  notmuch-config.c |   80 ++++++++++++++++++++----------------------------------
>  1 file changed, 29 insertions(+), 51 deletions(-)
>
> diff --git a/notmuch-config.c b/notmuch-config.c
> index befe9b5..305d213 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -496,6 +496,29 @@ notmuch_config_is_new (notmuch_config_t *config)
>      return config->is_new;
>  }
>  
> +static const char *
> +_config_get (notmuch_config_t *config, char **field,
> +	     const char *group, const char *key)
> +{
> +    if (*field == NULL) {
> +	char *value;
> +	value = g_key_file_get_string (config->key_file, group, key, NULL);
> +	if (value) {
> +	    *field = talloc_strdup (config, value);
> +	    free (value);
> +	}
> +    }
> +    return *field;
> +}
> +

The use of NULL here is clear enough in the context of the patch, but I
think it deserves a comment in the code; it is not obviously just code
movement after the patch is applied.

Re: [PATCH 2/2] cli: define config getters and setters using a macro

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> There's plenty of duplicated code in defining the functions for config
> get/set. Add macros to define the functions.
>
> ---
>
> This might be a bit too tricky for some people's tastes... let's see!
> ;)

I could go either way on this. I don't find it unbearably tricky; I'm
also not 100% sure it's needed. But if you and Tomi still like the idea,
I'm ok with it.

d