{"thread":{"id":"36812","subject":"[PATCH] string-list.h: Add a value to string_list initializer lists","startedAt":"2014-06-02T08:58:24Z","lastAt":"2014-06-02T20:00:23Z","messageCount":3,"participants":["Tanay Abhra","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"243125","messageId":"1401699504-29826-1-git-send-email-tanayabh@gmail.com","threadId":"36812","inReplyTo":null,"subject":"[PATCH] string-list.h: Add a value to string_list initializer lists","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-02T08:58:24Z","receivedAt":"2014-06-02T08:58:24Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Add a NULL value in the end of STRING_LIST_INIT_NODUP and\nSTRING_LIST_DUP to initialize `compare_strings_fn`.\n\nSigned-off-by: Tanay Abhra \n---\nWhen I used a malloced string_list to play around with string-list API and\nused the default init_list, it caused a seg fault. After an hour of debugging\nI saw that comapre_strings_fn should be initialized to NULL. In C, even an\nincomplete initialzer initializes every value to NULl or 0, so in normal\nusage in the codebase this problem never occurs. Still it is better to be\nthorough.\n\n string-list.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/string-list.h b/string-list.h\nindex de6769c..87ee419 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -15,8 +15,8 @@ struct string_list {\n \tcompare_strings_fn cmp; /* NULL uses strcmp() */\n };\n \n-#define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0 }\n-#define STRING_LIST_INIT_DUP { NULL, 0, 0, 1 }\n+#define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0, NULL }\n+#define STRING_LIST_INIT_DUP { NULL, 0, 0, 1, NULL }\n \n void print_string_list(const struct string_list *p, const char *text);\n void string_list_clear(struct string_list *list, int free_util);\n-- \n1.9.0.GIT\n"},{"id":"243126","messageId":"vpqa99vr37o.fsf@anie.imag.fr","threadId":"36812","inReplyTo":"1401699504-29826-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH] string-list.h: Add a value to string_list initializer lists","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-02T10:54:51Z","receivedAt":"2014-06-02T10:54:51Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> Add a NULL value in the end of STRING_LIST_INIT_NODUP and\n> STRING_LIST_DUP to initialize `compare_strings_fn`.\n>\n> Signed-off-by: Tanay Abhra \n> ---\n> When I used a malloced string_list to play around with string-list API and\n> used the default init_list, it caused a seg fault. After an hour of debugging\n> I saw that comapre_strings_fn should be initialized to NULL. In C, even an\n> incomplete initialzer initializes every value to NULl or 0, so in normal\n> usage in the codebase this problem never occurs. Still it is better to be\n> thorough.\n\nPart of this comment should be in the commit message itself. The \"I\nused ...\" part should not (it is your experience), but the last two\nsentences make sense IMHO.\n\nOther than that, the changes sounds good.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"243153","messageId":"xmqqlhtf3wvc.fsf@gitster.dls.corp.google.com","threadId":"36812","inReplyTo":"1401699504-29826-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH] string-list.h: Add a value to string_list initializer lists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-02T20:00:23Z","receivedAt":"2014-06-02T20:00:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra writes:\n\n> Add a NULL value in the end of STRING_LIST_INIT_NODUP and\n> STRING_LIST_DUP to initialize `compare_strings_fn`.\n\nHmph. That is \"what change is proposed\", which we can read from the\npatch text itself below. We would want to see \"why is this change a\ngood thing\" to justify it.\n\nAs you mentioned later \"In C, ...\", there is nothing wrong in what\nwe have (and we can not quite read what exactly you tried to do with\nuninitialized memory with these macros), so we need an excuse other\nthan correctness to justify this change. Perhaps like this?\n\n\tSTRING_LIST_INIT_{NODUP,DUP} initializers list values only\n\tfor earlier structure members, relying on the usual\n\tconvention in C that the omitted members are initailized to\n\t0, i.e. the former is expanded to the latter:\n\n\t struct string_list l = STRING_LIST_INIT_DUP;\n struct string_list l = { NULL, 0, 0, 1 };\n\n\tand the last member that is not mentioned (i.e. 'cmp') is\n\tinitialized to NULL.\n\n\tWhile there is nothing wrong in this construct, spelling out\n\tall the values where the macros are defined will serve also\n\tas a documentation, so let's do so.\n\n> Signed-off-by: Tanay Abhra \n> ---\n> When I used a malloced string_list to play around with string-list API and\n> used the default init_list, it caused a seg fault. After an hour of debugging\n> I saw that comapre_strings_fn should be initialized to NULL. In C, even an\n> incomplete initialzer initializes every value to NULl or 0, so in normal\n> usage in the codebase this problem never occurs. Still it is better to be\n> thorough.\n>\n> string-list.h | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/string-list.h b/string-list.h\n> index de6769c..87ee419 100644\n> --- a/string-list.h\n> +++ b/string-list.h\n> @@ -15,8 +15,8 @@ struct string_list {\n> \tcompare_strings_fn cmp; /* NULL uses strcmp() */\n> };\n> \n> -#define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0 }\n> -#define STRING_LIST_INIT_DUP { NULL, 0, 0, 1 }\n> +#define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0, NULL }\n> +#define STRING_LIST_INIT_DUP { NULL, 0, 0, 1, NULL }\n> \n> void print_string_list(const struct string_list *p, const char *text);\n> void string_list_clear(struct string_list *list, int free_util);\n"}]}