Re: [PATCH v2 1/2] string-list: Add string_list initializer helper functions

[Tanay Abhra <tanayabh@gmail.com>]

On 06/16/2014 03:59 PM, Junio C Hamano wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> When a compound construct like a string_list within another
>> struct is used, the default initializer macros are useless.
>> For such cases add helper functions for string_list
>> initialization for both DUP and NODUP modes.
>>
>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>> ---
> 
> 
> Sorry, but I do not understand the above "useless".  Do you mean to
> say that xyzzy below cannot be initialized that way?
> 
>  git.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/git.c b/git.c
> index d261575..17714d1 100644
> --- a/git.c
> +++ b/git.c
> @@ -595,11 +595,24 @@ static int run_argv(int *argcp, const char ***argv)
>  }
>  
>  
> +#include "string-list.h"
> +
>  int main(int argc, char **av)
>  {
>  	const char **argv = (const char **) av;
>  	const char *cmd;
>  
> +	struct compound {
> +		int frotz;
> +		struct string_list nitfol;
> +	} xyzzy = {
> +		314,
> +		STRING_LIST_INIT_DUP,
> +	};
> +	printf("dup-strings is set to %s\n",
> +	       xyzzy.nitfol.strdup_strings ? "true" : "false");
> +	return 0;
> +
>  	startup_info = &git_startup_info;
>  
>  	cmd = git_extract_argv0_path(argv[0]);
> 

I was actually explaining for cases like below,

+struct config_cache_entry {
+	struct hashmap_entry ent;
+	char *key;
+	struct string_list value_list;
+};

+static int config_cache_set_value(const char *key, const char *value)
+{
+	struct hashmap *config_cache;
+	struct config_cache_entry *e;
+
+	config_cache = get_config_cache();
+	e = config_cache_find_entry(key);
+	if (!e) {
+		e = xmalloc(sizeof(*e));
+		hashmap_entry_init(e, strhash(key));
+		e->key = xstrdup(key);
+		string_list_init_dup(&e->value_list);
+		string_list_append(&e->value_list, value);
+		hashmap_add(config_cache, e);
+	} else {
+		string_list_append(&e->value_list, value);
+	}
+	return 0;
+}

Here even if we use an initialization list as you have shown above, I would have to
check contents of 'struct hashmap_entry', thus totally breaking the encapsulation
that the string_list macro was providing. There may not be default values for
'struct hashmap_entry' as it may be using internal init function.

Also, I have to dynamically allocate the config_cache_entry struct, thus the
initialization list such as above cannot be used. Two previous reviewers of the
patch suggested I put a preparatory patch with string_list_init functions because
the default macros will be useless in my case.

Is there another way out?

Cheers,
Tanay abhra.