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

From: Tanay Abhra <tanayabh@gmail.com>
Date: Jun 17, 2014, 19:05 UTC
Message-ID: <53A09192.2030008@gmail.com>
In-Reply-To: <xmqqsin4o406.fsf@gitster.dls.corp.google.com>

On 06/16/2014 03:59 PM, Junio C Hamano wrote:

Show 47 quoted lines

> 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.