{"thread":{"id":"46757","subject":"[PATCH] for_each_string_list_item(): behave correctly for empty list","startedAt":"2017-09-15T16:00:50Z","lastAt":"2017-09-21T15:39:41Z","messageCount":29,"participants":["Michael Haggerty","Jonathan Nieder","SZEDER Gábor","Junio C Hamano","Stefan Beller","Kaartic Sivaraam","Andreas Schwab"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"328128","messageId":"cb2d4d71c7c1db452b86c8076c153cabe7384e28.1505490776.git.mhagger@alum.mit.edu","threadId":"46757","inReplyTo":null,"subject":"[PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-15T16:00:38Z","receivedAt":"2017-09-15T16:00:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If you pass a newly-initialized or newly-cleared `string_list` to\n`for_each_string_list_item()`, then the latter does\n\n    for (\n            item = (list)->items; /* note, this is NULL */\n            item < (list)->items + (list)->nr; /* note: NULL + 0 */\n            ++item)\n\nEven though this probably works almost everywhere, it is undefined\nbehavior, and it could plausibly cause highly-optimizing compilers to\nmisbehave.\n\nIt would be a pain to have to change the signature of this macro, and\nwe'd prefer not to add overhead to each iteration of the loop. So\ninstead, whenever `list->items` is NULL, initialize `item` to point at\na dummy `string_list_item` created for the purpose.\n\nThis problem was noticed by Coverity.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\nJust a little thing I noticed in a Coverity report. This macro has\nbeen broken since it was first introduced, in 2010.\n\nThis patch applies against maint. It is also available from my Git\nfork [1] as branch `iter-empty-string-list`.\n\nMichael\n\n[1] https://github.com/mhagger/git\n\n string-list.c | 2 ++\n string-list.h | 7 +++++--\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex 806b4c8723..7eacf6037f 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -1,6 +1,8 @@\n #include \"cache.h\"\n #include \"string-list.h\"\n \n+struct string_list_item dummy_string_list_item;\n+\n void string_list_init(struct string_list *list, int strdup_strings)\n {\n \tmemset(list, 0, sizeof(*list));\ndiff --git a/string-list.h b/string-list.h\nindex 29bfb7ae45..79bb78d80a 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -32,8 +32,11 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n typedef int (*string_list_each_func_t)(struct string_list_item *, void *);\n int for_each_string_list(struct string_list *list,\n \t\t\t string_list_each_func_t, void *cb_data);\n-#define for_each_string_list_item(item,list) \\\n-\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n+extern struct string_list_item dummy_string_list_item;\n+#define for_each_string_list_item(item,list)                                 \\\n+\tfor (item = (list)->items ? (list)->items : &dummy_string_list_item; \\\n+\t     item < (list)->items + (list)->nr;                              \\\n+\t     ++item)\n \n /*\n  * Apply want to each item in list, retaining only the ones for which\n-- \n2.14.1\n\n"},{"id":"328142","messageId":"20170915184323.GU27425@aiede.mtv.corp.google.com","threadId":"46757","inReplyTo":"cb2d4d71c7c1db452b86c8076c153cabe7384e28.1505490776.git.mhagger@alum.mit.edu","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-15T18:43:23Z","receivedAt":"2017-09-15T18:43:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMichael Haggerty wrote:\n\n> If you pass a newly-initialized or newly-cleared `string_list` to\n> `for_each_string_list_item()`, then the latter does\n>\n>     for (\n>             item = (list)->items; /* note, this is NULL */\n>             item < (list)->items + (list)->nr; /* note: NULL + 0 */\n>             ++item)\n>\n> Even though this probably works almost everywhere, it is undefined\n> behavior, and it could plausibly cause highly-optimizing compilers to\n> misbehave.\n\nWait, NULL + 0 is undefined behavior?\n\n*checks the standard*  C99 section 6.5.6.8 says\n\n\t\"If both the pointer operand and the result point to elements\n\tof the same array object, or one past the last element of the\n\tarray object, the evaluation shall not produce an overflow;\n\totherwise, the behavior is undefined.\"\n\nC99 section 7.17.3 says\n\n\t\"NULL\n\n\twhich expands to an implementation-defined null pointer constant\"\n\n6.3.2.3.3 says\n\n\t\"An integer constant expression with the value 0, or such an\n\texpression cast to type void *, is called a null pointer\n\tconstant.  If a null pointer constant is converted to a\n\tpointer type, the resulting pointer, called a null pointer, is\n\tguaranteed to compare unequal to a pointer to any object or\n\tfunction.\"\n\nNULL doesn't point to anything so it looks like adding 0 to a null\npointer is indeed undefined.  (As a piece of trivia, strictly speaking\nNULL + 0 would be undefined on some implementations and defined on\nothers, since an implementation is permitted to #define NULL to 0.)\n\nSo Coverity is not just warning because it is not able to guarantee\nthat list->nr is 0.  Huh.\n\n> It would be a pain to have to change the signature of this macro, and\n> we'd prefer not to add overhead to each iteration of the loop. So\n> instead, whenever `list->items` is NULL, initialize `item` to point at\n> a dummy `string_list_item` created for the purpose.\n\nWhat signature change do you mean?  I don't understand what this\nparagraph is alluding to.\n\n> This problem was noticed by Coverity.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n[...]\n>  string-list.c | 2 ++\n>  string-list.h | 7 +++++--\n>  2 files changed, 7 insertions(+), 2 deletions(-)\n\nDoes the following alternate fix work?  I think I prefer it because\nit doesn't require introducing a new global.\n\nThanks,\nJonathan\n\ndiff --git i/string-list.h w/string-list.h\nindex 29bfb7ae45..dae33fbb89 100644\n--- i/string-list.h\n+++ w/string-list.h\n@@ -33,7 +33,9 @@ typedef int (*string_list_each_func_t)(struct string_list_item *, void *);\n int for_each_string_list(struct string_list *list,\n \t\t\t string_list_each_func_t, void *cb_data);\n #define for_each_string_list_item(item,list) \\\n-\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n+\tfor (item = (list)->items; \\\n+\t     (list)->items && item < (list)->items + (list)->nr; \\\n+\t     ++item)\n \n /*\n  * Apply want to each item in list, retaining only the ones for which\n"},{"id":"328180","messageId":"b8951886-feab-a87a-9683-3c155cfa98a8@alum.mit.edu","threadId":"46757","inReplyTo":"20170915184323.GU27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-16T04:06:34Z","receivedAt":"2017-09-16T04:06:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/15/2017 08:43 PM, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n>> If you pass a newly-initialized or newly-cleared `string_list` to\n>> `for_each_string_list_item()`, then the latter does\n>>\n>>     for (\n>>             item = (list)->items; /* note, this is NULL */\n>>             item < (list)->items + (list)->nr; /* note: NULL + 0 */\n>>             ++item)\n>>\n>> Even though this probably works almost everywhere, it is undefined\n>> behavior, and it could plausibly cause highly-optimizing compilers to\n>> misbehave.\n> \n> Wait, NULL + 0 is undefined behavior?\n> \n> *checks the standard*  [...]\n> NULL doesn't point to anything so it looks like adding 0 to a null\n> pointer is indeed undefined.\n\nThanks for the legal work :-)\n\n>                             (As a piece of trivia, strictly speaking\n> NULL + 0 would be undefined on some implementations and defined on\n> others, since an implementation is permitted to #define NULL to 0.)\n\nIsn't that the very definition of \"undefined behavior\", in the sense of\na language standard?\n\n> [...]\n>> It would be a pain to have to change the signature of this macro, and\n>> we'd prefer not to add overhead to each iteration of the loop. So\n>> instead, whenever `list->items` is NULL, initialize `item` to point at\n>> a dummy `string_list_item` created for the purpose.\n> \n> What signature change do you mean?  I don't understand what this\n> paragraph is alluding to.\n\nI was thinking that one solution would be for the caller to provide a\n`size_t` variable for the macro's use as a counter (since I don't see a\nway for the macro to declare its own counter). The options are pretty\nlimited because whatever the macro expands to has to play the same\nsyntactic role as `for (...; ...; ...)`.\n\n> [...]\n> Does the following alternate fix work?  I think I prefer it because\n> it doesn't require introducing a new global. [...]\n>  #define for_each_string_list_item(item,list) \\\n> -\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n> +\tfor (item = (list)->items; \\\n> +\t     (list)->items && item < (list)->items + (list)->nr; \\\n> +\t     ++item)\n\nThis is the possibility that I was referring to as \"add[ing] overhead to\neach iteration of the loop\". I'd rather not add an extra test-and-branch\nto every iteration of a loop in which `list->items` is *not* NULL, which\nyour solution appears to do. Or are compilers routinely able to optimize\nthe check out?\n\nThe new global might be aesthetically unpleasant, but it only costs two\nwords of memory, so I don't see it as a big disadvantage.\n\nAnother, more invasive change would be to initialize\n`string_list::items` to *always* point at `dummy_string_list_item`,\nrather similar to how `strbuf_slopbuf` is pointed at by empty `strbuf`s.\nBut I really don't think the effort would be justified.\n\nMichael\n"},{"id":"328230","messageId":"20170916115118.15490-1-szeder.dev@gmail.com","threadId":"46757","inReplyTo":"b8951886-feab-a87a-9683-3c155cfa98a8@alum.mit.edu","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-09-16T11:51:18Z","receivedAt":"2017-09-16T11:52:04Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\n> >> It would be a pain to have to change the signature of this macro, and\n> >> we'd prefer not to add overhead to each iteration of the loop. So\n> >> instead, whenever `list->items` is NULL, initialize `item` to point at\n> >> a dummy `string_list_item` created for the purpose.\n> > \n> > What signature change do you mean?  I don't understand what this\n> > paragraph is alluding to.\n> \n> I was thinking that one solution would be for the caller to provide a\n> `size_t` variable for the macro's use as a counter (since I don't see a\n> way for the macro to declare its own counter). The options are pretty\n> limited because whatever the macro expands to has to play the same\n> syntactic role as `for (...; ...; ...)`.\n\nAnother option to consider is to squeeze in an if-else before the for\nloop header to handle the empty list case like this:\n\ndiff --git a/string-list.h b/string-list.h\nindex 29bfb7ae4..9eed47de0 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -32,8 +32,11 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n typedef int (*string_list_each_func_t)(struct string_list_item *, void *);\n int for_each_string_list(struct string_list *list,\n \t\t\t string_list_each_func_t, void *cb_data);\n-#define for_each_string_list_item(item,list) \\\n-\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n+#define for_each_string_list_item(item,list) \t\\\n+\tif ((list)->items == NULL) {\t\t\\\n+\t\t/* empty list, do nothing */\t\\\n+\t} else\t\t\t\t\t\\\n+\t\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n \n /*\n  * Apply want to each item in list, retaining only the ones for which\n\nThis way there would be neither additional overhead in each iteration\nnor a new global.\n\nAlas, there is a catch.  We can't use curly braces in the macro's else\nbranch, because the macro would contain only the opening brace but not\nthe closing one, which must come after the end of the loop's body.\nThis means that the modified macro couldn't be used in if-else\nbranches which themselves don't have curly braces, because it causes\nambiguity:\n\n  if (condition)\n      for_each_string_list_item(item, list)\n          a_simple_oneliner(item);\n\nOur coding guidelines encourage this style for one-liner loop bodies,\nand there is indeed one such place in our codebase, so the following\nhunk is needed as well:\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 11d6f3d98..00fa1622f 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -295,9 +295,10 @@ static int generate_push_cert(struct strbuf *req_buf,\n \t}\n \tif (push_cert_nonce[0])\n \t\tstrbuf_addf(&cert, \"nonce %s\\n\", push_cert_nonce);\n-\tif (args->push_options)\n+\tif (args->push_options) {\n \t\tfor_each_string_list_item(item, args->push_options)\n \t\t\tstrbuf_addf(&cert, \"push-option %s\\n\", item->string);\n+\t}\n \tstrbuf_addstr(&cert, \"\\n\");\n \n \tfor (ref = remote_refs; ref; ref = ref->next) {\n\n\nLuckily, reasonably modern compilers warn about such ambiguity, so\nperhaps this is an acceptable compromise?\n\n\n> > [...]\n> > Does the following alternate fix work?  I think I prefer it because\n> > it doesn't require introducing a new global. [...]\n> >  #define for_each_string_list_item(item,list) \\\n> > -\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n> > +\tfor (item = (list)->items; \\\n> > +\t     (list)->items && item < (list)->items + (list)->nr; \\\n> > +\t     ++item)\n> \n> This is the possibility that I was referring to as \"add[ing] overhead to\n> each iteration of the loop\". I'd rather not add an extra test-and-branch\n> to every iteration of a loop in which `list->items` is *not* NULL, which\n> your solution appears to do. Or are compilers routinely able to optimize\n> the check out?\n> \n> The new global might be aesthetically unpleasant, but it only costs two\n> words of memory, so I don't see it as a big disadvantage.\n> \n> Another, more invasive change would be to initialize\n> `string_list::items` to *always* point at `dummy_string_list_item`,\n> rather similar to how `strbuf_slopbuf` is pointed at by empty `strbuf`s.\n> But I really don't think the effort would be justified.\n\n"},{"id":"328238","messageId":"xmqqefr6uolr.fsf@gitster.mtv.corp.google.com","threadId":"46757","inReplyTo":"cb2d4d71c7c1db452b86c8076c153cabe7384e28.1505490776.git.mhagger@alum.mit.edu","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-17T00:59:28Z","receivedAt":"2017-09-17T00:59:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> If you pass a newly-initialized or newly-cleared `string_list` to\n> `for_each_string_list_item()`, then the latter does\n>\n>     for (\n>             item = (list)->items; /* note, this is NULL */\n>             item < (list)->items + (list)->nr; /* note: NULL + 0 */\n>             ++item)\n>\n> Even though this probably works almost everywhere, it is undefined\n> behavior, and it could plausibly cause highly-optimizing compilers to\n> misbehave.\n> ...\n> It would be a pain to have to change the signature of this macro, and\n> we'd prefer not to add overhead to each iteration of the loop. So\n> instead, whenever `list->items` is NULL, initialize `item` to point at\n> a dummy `string_list_item` created for the purpose.\n> ...\n> -#define for_each_string_list_item(item,list) \\\n> -\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n> +extern struct string_list_item dummy_string_list_item;\n> +#define for_each_string_list_item(item,list)                                 \\\n> +\tfor (item = (list)->items ? (list)->items : &dummy_string_list_item; \\\n> +\t     item < (list)->items + (list)->nr;                              \\\n> +\t     ++item)\n\nSorry, but I am confused.\n\nSo when (list)->items is NULL, the loop termination condition that\nused to be\n\n\tNULL < NULL + 0\n\nthat was problematic because NULL + 0 is problematic now becomes\n\n\t&dummy < NULL + 0\n\nin the new code?  What made NULL + 0 not problematic now?\n"},{"id":"328259","messageId":"9d4eb543-7abc-abf5-ed14-73ee75d87547@alum.mit.edu","threadId":"46757","inReplyTo":"20170916115118.15490-1-szeder.dev@gmail.com","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-17T10:19:43Z","receivedAt":"2017-09-17T10:19:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/16/2017 01:51 PM, SZEDER Gábor wrote:\n>>>> It would be a pain to have to change the signature of this macro, and\n>>>> we'd prefer not to add overhead to each iteration of the loop. So\n>>>> instead, whenever `list->items` is NULL, initialize `item` to point at\n>>>> a dummy `string_list_item` created for the purpose.\n>>>\n>>> What signature change do you mean?  I don't understand what this\n>>> paragraph is alluding to.\n>>\n>> I was thinking that one solution would be for the caller to provide a\n>> `size_t` variable for the macro's use as a counter (since I don't see a\n>> way for the macro to declare its own counter). The options are pretty\n>> limited because whatever the macro expands to has to play the same\n>> syntactic role as `for (...; ...; ...)`.\n> \n> Another option to consider is to squeeze in an if-else before the for\n> loop header to handle the empty list case like this:\n> \n> diff --git a/string-list.h b/string-list.h\n> index 29bfb7ae4..9eed47de0 100644\n> --- a/string-list.h\n> +++ b/string-list.h\n> @@ -32,8 +32,11 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n>  typedef int (*string_list_each_func_t)(struct string_list_item *, void *);\n>  int for_each_string_list(struct string_list *list,\n>  \t\t\t string_list_each_func_t, void *cb_data);\n> -#define for_each_string_list_item(item,list) \\\n> -\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n> +#define for_each_string_list_item(item,list) \t\\\n> +\tif ((list)->items == NULL) {\t\t\\\n> +\t\t/* empty list, do nothing */\t\\\n> +\t} else\t\t\t\t\t\\\n> +\t\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n>  \n>  /*\n>   * Apply want to each item in list, retaining only the ones for which\n> \n> This way there would be neither additional overhead in each iteration\n> nor a new global.\n> \n> Alas, there is a catch.  We can't use curly braces in the macro's else\n> branch, because the macro would contain only the opening brace but not\n> the closing one, which must come after the end of the loop's body.\n> This means that the modified macro couldn't be used in if-else\n> branches which themselves don't have curly braces, because it causes\n> ambiguity:\n> \n>   if (condition)\n>       for_each_string_list_item(item, list)\n>           a_simple_oneliner(item);\n\nIt's not ambiguous as far as the language standard is concerned. The\nlatter is clear that an `else` binds to the nearest `if`. The problem is\nthat this is a common programmer error, so compilers \"helpfully\" warn\nabout it even though it would do exactly what we want.\n\n> Our coding guidelines encourage this style for one-liner loop bodies,\n> and there is indeed one such place in our codebase, so the following\n> hunk is needed as well:\n> \n> diff --git a/send-pack.c b/send-pack.c\n> index 11d6f3d98..00fa1622f 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -295,9 +295,10 @@ static int generate_push_cert(struct strbuf *req_buf,\n>  \t}\n>  \tif (push_cert_nonce[0])\n>  \t\tstrbuf_addf(&cert, \"nonce %s\\n\", push_cert_nonce);\n> -\tif (args->push_options)\n> +\tif (args->push_options) {\n>  \t\tfor_each_string_list_item(item, args->push_options)\n>  \t\t\tstrbuf_addf(&cert, \"push-option %s\\n\", item->string);\n> +\t}\n>  \tstrbuf_addstr(&cert, \"\\n\");\n>  \n>  \tfor (ref = remote_refs; ref; ref = ref->next) {\n> \n> \n> Luckily, reasonably modern compilers warn about such ambiguity, so\n> perhaps this is an acceptable compromise?\n\nThis change kindof goes *against* our coding guidelines, so it's not\nideal either, but I suppose we could probably live with it.\n\nMichael\n"},{"id":"328260","messageId":"5c86b55e-20f6-df8e-b01f-66876c3a5f46@alum.mit.edu","threadId":"46757","inReplyTo":"xmqqefr6uolr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-17T10:24:34Z","receivedAt":"2017-09-17T10:24:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/17/2017 02:59 AM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> If you pass a newly-initialized or newly-cleared `string_list` to\n>> `for_each_string_list_item()`, then the latter does\n>>\n>>     for (\n>>             item = (list)->items; /* note, this is NULL */\n>>             item < (list)->items + (list)->nr; /* note: NULL + 0 */\n>>             ++item)\n>>\n>> Even though this probably works almost everywhere, it is undefined\n>> behavior, and it could plausibly cause highly-optimizing compilers to\n>> misbehave.\n>> ...\n>> It would be a pain to have to change the signature of this macro, and\n>> we'd prefer not to add overhead to each iteration of the loop. So\n>> instead, whenever `list->items` is NULL, initialize `item` to point at\n>> a dummy `string_list_item` created for the purpose.\n>> ...\n>> -#define for_each_string_list_item(item,list) \\\n>> -\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n>> +extern struct string_list_item dummy_string_list_item;\n>> +#define for_each_string_list_item(item,list)                                 \\\n>> +\tfor (item = (list)->items ? (list)->items : &dummy_string_list_item; \\\n>> +\t     item < (list)->items + (list)->nr;                              \\\n>> +\t     ++item)\n> \n> Sorry, but I am confused.\n> \n> So when (list)->items is NULL, the loop termination condition that\n> used to be\n> \n> \tNULL < NULL + 0\n> \n> that was problematic because NULL + 0 is problematic now becomes\n> \n> \t&dummy < NULL + 0\n> \n> in the new code?  What made NULL + 0 not problematic now?\n\n*sigh* of course you're right. I should know better than to \"fire off a\nquick fix to the mailing list\".\n\nI guess the two proposals that are still in the running for rescuing\nthis macro are Jonathan's and Gábor's. I have no strong preference\neither way.\n\nMichael\n"},{"id":"328280","messageId":"xmqqfubku9iy.fsf@gitster.mtv.corp.google.com","threadId":"46757","inReplyTo":"5c86b55e-20f6-df8e-b01f-66876c3a5f46@alum.mit.edu","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-18T00:37:25Z","receivedAt":"2017-09-18T00:41:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> *sigh* of course you're right. I should know better than to \"fire off a\n> quick fix to the mailing list\".\n>\n> I guess the two proposals that are still in the running for rescuing\n> this macro are Jonathan's and Gábor's. I have no strong preference\n> either way.\n\nIf somebody is writing this outisde a macro as a one-shot thing, the\nmost natural and readable way I would imagine would be\n\n\tif (the list is empty)\n        \t;\n\telse\n\t\tfor (each item in the list)\n\t\t\twork on item\n\nI would think.  That \"work on item\" part may not be a single\nexpression statement and instead be a compound statement inside a\npair of braces {}.  Making a shorter version, i.e.\n\n\tif (!the list is empty)\n\t\tfor (each item in the list)\n\t\t\twork on item\n\ninto a macro probably has syntax issues around cascading if/else\nchain, e.g.\n\n\tif (condition caller cares about)\n\t\tfor_each_string_list_item() {\n\t\t\tdo this thing\n\t\t}\n\telse\n\t\tdo something else\n\nwould expand to\n\n\tif (condition caller cares about)\n\t\tif (!the list is empty)\n\t\t\tfor (each item in the list) {\n\t\t\t\tdo this thing\n\t\t\t}\n\telse\n\t\tdo something else\n\nwhich is wrong.  But I couldn't think of a way to break the longer\none with the body of the macro in the \"else\" clause in a similar\nway.  An overly helpful compiler might say\n\n\tif (condition caller cares about)\n\t\tif (the list is empty)\n\t\t\t;\n\t\telse\n\t\t\tfor (each item in the list) {\n\t\t\t\tdo this thing\n\t\t\t}\n\telse\n\t\tdo something else\n\nthat it wants a pair of {} around the then-clause of the outer if;\nif we can find a way to squelch such warnings only with this\nconstruct that comes from the macro, then this solution may be ideal.\n\nIf we cannot do that, then\n\n\tfor (item = (list)->items; /* could be NULL */\n\t     (list)->items && item < (list)->items + (list)->nr;\n\t     item++)\n\t\twork on item\n\nmay be an obvious way to write it without any such syntax worries,\nbut I am unclear how a \"undefined behaviour\" contaminate the code\naround it.  My naive reading of the termination condition of the\nabove is:\n\n\t\"(list)->items &&\" clearly means that (list)->items is not\n\tNULL in what follows it, i.e. (list->items + (list)->nr\n\tcannot be a NULL + 0, so we are not allowed to make demon\n\tfly out of your nose.\n\nbut I wonder if this alternative reading is allowed:\n\n\t(list)->items is not assigned to in this expression and is\n\tused in a subexpression \"(list)->items + (list)->nr\" here;\n\tfor that subexpression not to be \"undefined\", it cannot be\n\tNULL, so we can optimize out \"do this only (list)->items is\n\tnot NULL\" part.\n\nwhich takes us back to where we started X-<.  So I dunno.\n\nI am hoping that this last one is not allowed and we can use the\n\"same condition is checked every time we loop\" version that hides\nthe uglyness inside the macro.\n"},{"id":"328324","messageId":"CAGZ79kYXDhcVXd2C-x6e=o7jYdKqV22DY45c7E2TeuhKLfn26w@mail.gmail.com","threadId":"46757","inReplyTo":"xmqqfubku9iy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-19T00:08:13Z","receivedAt":"2017-09-19T00:08:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> I am hoping that this last one is not allowed and we can use the\n> \"same condition is checked every time we loop\" version that hides\n> the uglyness inside the macro.\n\nBy which you are referring to Jonathans solution posted.\nMaybe we can combine the two solutions (checking for thelist\nto not be NULL once, by Jonathan) and using an outer structure\n(SZEDERs solution) by replacing the condition by a for loop,\nroughly (untested):\n\n#define for_each_string_list_item(item,list) \\\n-       for (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n+    for (; list; list = NULL)\n+        for (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n\nas that would not mingle with any dangling else clause.\nIt is also just one statement, such that\n\n    if (bla)\n      for_each_string_list_item {\n        baz(item);\n      }\n    else\n      foo;\n\nstill works.\n\nAre there downsides to this combined approach?\n"},{"id":"328370","messageId":"dab2d555-7e09-4eb3-19b8-cab085626bbe@alum.mit.edu","threadId":"46757","inReplyTo":"CAGZ79kYXDhcVXd2C-x6e=o7jYdKqV22DY45c7E2TeuhKLfn26w@mail.gmail.com","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-19T06:51:22Z","receivedAt":"2017-09-19T06:51:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/19/2017 02:08 AM, Stefan Beller wrote:\n>> I am hoping that this last one is not allowed and we can use the\n>> \"same condition is checked every time we loop\" version that hides\n>> the uglyness inside the macro.\n> \n> By which you are referring to Jonathans solution posted.\n> Maybe we can combine the two solutions (checking for thelist\n> to not be NULL once, by Jonathan) and using an outer structure\n> (SZEDERs solution) by replacing the condition by a for loop,\n> roughly (untested):\n> \n> #define for_each_string_list_item(item,list) \\\n> -       for (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n> +    for (; list; list = NULL)\n> +        for (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n> \n> as that would not mingle with any dangling else clause.\n> It is also just one statement, such that\n> \n>     if (bla)\n>       for_each_string_list_item {\n>         baz(item);\n>       }\n>     else\n>       foo;\n> \n> still works.\n> \n> Are there downsides to this combined approach?\n\nOn the plus side, it's pleasantly devious; I wouldn't have thought of\nusing a `for` loop for the initial test. But it doesn't work as written,\nbecause (1) we don't need to guard against `list` being NULL, but rather\n`list->items`; and (2) we don't have the liberty to set `list = NULL`\n(or `list->items = NULL`, because `list` is owned by the caller and we\nshouldn't modify it.\n\nThe following is a bit closer:\n\n#define for_each_string_list_item(item,list) \\\n\tfor (item = (list)->items; item; item = NULL) \\\n        \tfor (; item < (list)->items + (list)->nr; ++item)\n\nBut I think that also fails, because a callsite that does\n\n\tfor_each_string_list_item(myitem, mylist)\n\t\tif (myitem.util)\n\t\t\tbreak;\n\nwould expect that `myitem` is still set after breaking out of the loop,\nwhereas the outer `for` loop would reset it to NULL.\n\nIf `break` were an expression we could do something like\n\n#define for_each_string_list_item(item,list) \\\n\tfor (item = (list)->items; item; break) \\\n        \tfor (; item < (list)->items + (list)->nr; ++item)\n\nSo I think we're still left with the suggestions of Jonathan or Gábor.\nOr the bigger change of initializing `string_list::items` to point at an\nempty sentinal array (similar to `strbuf_slopbuf`) rather than NULL.\nPersonally, I think that Jonathan's approach makes the most sense,\nunless somebody wants to jump in an implement a `string_list_slopbuf`.\n\nBy the way, I wonder if any open-coded loops over `string_lists` make\nthe same mistake as the macro?\n\nMichael\n"},{"id":"328384","messageId":"CAM0VKjn=KjTHBoubJKbxx7MasJ6wWcUFrCwrvr5oHwUCsfr_Pw@mail.gmail.com","threadId":"46757","inReplyTo":"dab2d555-7e09-4eb3-19b8-cab085626bbe@alum.mit.edu","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-09-19T13:38:00Z","receivedAt":"2017-09-19T13:38:07Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Sep 19, 2017 at 8:51 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 09/19/2017 02:08 AM, Stefan Beller wrote:\n>>> I am hoping that this last one is not allowed and we can use the\n>>> \"same condition is checked every time we loop\" version that hides\n>>> the uglyness inside the macro.\n>>\n>> By which you are referring to Jonathans solution posted.\n>> Maybe we can combine the two solutions (checking for thelist\n>> to not be NULL once, by Jonathan) and using an outer structure\n>> (SZEDERs solution) by replacing the condition by a for loop,\n>> roughly (untested):\n>>\n>> #define for_each_string_list_item(item,list) \\\n>> -       for (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n>> +    for (; list; list = NULL)\n>> +        for (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n>>\n>> as that would not mingle with any dangling else clause.\n>> It is also just one statement, such that\n>>\n>>     if (bla)\n>>       for_each_string_list_item {\n>>         baz(item);\n>>       }\n>>     else\n>>       foo;\n>>\n>> still works.\n>>\n>> Are there downsides to this combined approach?\n>\n> On the plus side, it's pleasantly devious; I wouldn't have thought of\n> using a `for` loop for the initial test. But it doesn't work as written,\n> because (1) we don't need to guard against `list` being NULL, but rather\n> `list->items`; and (2) we don't have the liberty to set `list = NULL`\n> (or `list->items = NULL`, because `list` is owned by the caller and we\n> shouldn't modify it.\n>\n> The following is a bit closer:\n>\n> #define for_each_string_list_item(item,list) \\\n>         for (item = (list)->items; item; item = NULL) \\\n>                 for (; item < (list)->items + (list)->nr; ++item)\n>\n> But I think that also fails, because a callsite that does\n>\n>         for_each_string_list_item(myitem, mylist)\n>                 if (myitem.util)\n>                         break;\n>\n> would expect that `myitem` is still set after breaking out of the loop,\n> whereas the outer `for` loop would reset it to NULL.\n>\n> If `break` were an expression we could do something like\n>\n> #define for_each_string_list_item(item,list) \\\n>         for (item = (list)->items; item; break) \\\n>                 for (; item < (list)->items + (list)->nr; ++item)\n\nA bit \"futuristic\" option along these lines could be something like\nthis, using a scoped loop variable in the outer loop to ensure that\nit's executed at most once:\n\n  #define for_each_string_list_item(item,list) \\\n      for (int f_e_s_l_i = 1; (list)->items && f_e_s_l_i; f_e_s_l_i = 0) \\\n          for (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n\nThe high number of underscores are an attempt to make reasonably sure\nthat the macro's loop variable doesn't shadow any variable in its\ncallers or isn't being shadowed in the loop body, which might(?)\ntrigger warnings in some compilers.\n\nAlas we don't allow scoping the loop variable in for loops, and even a\ntest balloon patch didn't make it into git.git.\n\n  https://public-inbox.org/git/20170719181956.15845-1-sbeller@google.com/T/#u\n\n\n> So I think we're still left with the suggestions of Jonathan or Gábor.\n> Or the bigger change of initializing `string_list::items` to point at an\n> empty sentinal array (similar to `strbuf_slopbuf`) rather than NULL.\n> Personally, I think that Jonathan's approach makes the most sense,\n> unless somebody wants to jump in an implement a `string_list_slopbuf`.\n>\n> By the way, I wonder if any open-coded loops over `string_lists` make\n> the same mistake as the macro?\n"},{"id":"328385","messageId":"CAM0VKjmuyp0pn29s8=diCpA0FPjjFJH-LcxQmOJH6Z-7WZNurA@mail.gmail.com","threadId":"46757","inReplyTo":"CAM0VKjn=KjTHBoubJKbxx7MasJ6wWcUFrCwrvr5oHwUCsfr_Pw@mail.gmail.com","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-09-19T13:45:23Z","receivedAt":"2017-09-19T13:45:29Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Sep 19, 2017 at 3:38 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> A bit \"futuristic\" option along these lines could be something like\n> this, using a scoped loop variable in the outer loop to ensure that\n> it's executed at most once:\n>\n>   #define for_each_string_list_item(item,list) \\\n>       for (int f_e_s_l_i = 1; (list)->items && f_e_s_l_i; f_e_s_l_i = 0) \\\n>           for (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n>\n> The high number of underscores are an attempt to make reasonably sure\n> that the macro's loop variable doesn't shadow any variable in its\n> callers or isn't being shadowed in the loop body, which might(?)\n> trigger warnings in some compilers.\n\nWell, and a poor attempt at that, because, of course, the loop\nvariable would still be shadowed in nested for_each_string_list_item\nloops...  And our codebase has these loops nested in\nentry.c:finish_delayed_checkout().\n\nOTOH, we don't seem to care too much about shadowed variables, since\nbuilding with -Wshadow gives 91 warnings in current master...\n"},{"id":"328387","messageId":"b03c7b09-853f-a2ed-f73e-7d946c90cedb@gmail.com","threadId":"46757","inReplyTo":"b8951886-feab-a87a-9683-3c155cfa98a8@alum.mit.edu","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"Kaartic Sivaraam","fromEmail":"kaarticsivaraam91196@gmail.com","sentAt":"2017-09-19T14:38:06Z","receivedAt":"2017-09-19T14:38:36Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Saturday 16 September 2017 09:36 AM, Michael Haggerty wrote:\n>> Does the following alternate fix work?  I think I prefer it because\n>> it doesn't require introducing a new global. [...]\n>>   #define for_each_string_list_item(item,list) \\\n>> -\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n>> +\tfor (item = (list)->items; \\\n>> +\t     (list)->items && item < (list)->items + (list)->nr; \\\n>> +\t     ++item)\n> This is the possibility that I was referring to as \"add[ing] overhead to\n> each iteration of the loop\". I'd rather not add an extra test-and-branch\n> to every iteration of a loop in which `list->items` is *not* NULL, which\n> your solution appears to do. Or are compilers routinely able to optimize\n> the check out?\n\nIt seems at least 'gcc' is able to optimize this out even with a -O1\nand 'clang' optimizes this out with a -O2. Taking a sneak peek at\nthe 'Makefile' shows that our default is -O2.\n\nFor a proof, see https://godbolt.org/g/CPt73L\n\n---\nKaartic\n"},{"id":"328434","messageId":"xmqq4lryqhcw.fsf@gitster.mtv.corp.google.com","threadId":"46757","inReplyTo":"b03c7b09-853f-a2ed-f73e-7d946c90cedb@gmail.com","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-20T01:38:39Z","receivedAt":"2017-09-20T01:38:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:\n\n> On Saturday 16 September 2017 09:36 AM, Michael Haggerty wrote:\n>>> Does the following alternate fix work?  I think I prefer it because\n>>> it doesn't require introducing a new global. [...]\n>>>   #define for_each_string_list_item(item,list) \\\n>>> -\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n>>> +\tfor (item = (list)->items; \\\n>>> +\t     (list)->items && item < (list)->items + (list)->nr; \\\n>>> +\t     ++item)\n>> This is the possibility that I was referring to as \"add[ing] overhead to\n>> each iteration of the loop\". I'd rather not add an extra test-and-branch\n>> to every iteration of a loop in which `list->items` is *not* NULL, which\n>> your solution appears to do. Or are compilers routinely able to optimize\n>> the check out?\n>\n> It seems at least 'gcc' is able to optimize this out even with a -O1\n> and 'clang' optimizes this out with a -O2. Taking a sneak peek at\n> the 'Makefile' shows that our default is -O2.\n\nBut doesn't the versions of gcc and clang currently available do the\nright thing with the current code without this change anyway?  I've\nbeen operating under the assumption that this is to future-proof the\ncode even when the compilers change to use the \"NULL+0 is undefined\"\nas an excuse to make demons fly out of your nose, so unfortunately I\ndo not think it is not so huge a plus to find that the current\ncompilers do the right thing to the code with proposed updates.\n\n"},{"id":"328435","messageId":"20170920014305.GA126984@aiede.mtv.corp.google.com","threadId":"46757","inReplyTo":"xmqq4lryqhcw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-20T01:43:05Z","receivedAt":"2017-09-20T01:43:35Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n> Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:\n>> On Saturday 16 September 2017 09:36 AM, Michael Haggerty wrote:\n>>> Jonathan Nieder wrote:\n\n>>>> Does the following alternate fix work?  I think I prefer it because\n>>>> it doesn't require introducing a new global. [...]\n>>>>   #define for_each_string_list_item(item,list) \\\n>>>> -\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n>>>> +\tfor (item = (list)->items; \\\n>>>> +\t     (list)->items && item < (list)->items + (list)->nr; \\\n>>>> +\t     ++item)\n>>>\n>>> This is the possibility that I was referring to as \"add[ing] overhead to\n>>> each iteration of the loop\". I'd rather not add an extra test-and-branch\n>>> to every iteration of a loop in which `list->items` is *not* NULL, which\n>>> your solution appears to do. Or are compilers routinely able to optimize\n>>> the check out?\n>>\n>> It seems at least 'gcc' is able to optimize this out even with a -O1\n>> and 'clang' optimizes this out with a -O2. Taking a sneak peek at\n>> the 'Makefile' shows that our default is -O2.\n>\n> But doesn't the versions of gcc and clang currently available do the\n> right thing with the current code without this change anyway?  I've\n> been operating under the assumption that this is to future-proof the\n> code even when the compilers change to use the \"NULL+0 is undefined\"\n> as an excuse to make demons fly out of your nose, so unfortunately I\n> do not think it is not so huge a plus to find that the current\n> compilers do the right thing to the code with proposed updates.\n\nI think you and Kaartic are talking about different things.  Kaartic\nwas checking that this wouldn't introduce a performance regression\n(thanks!).  You are concerned about whether the C standard and common\npractice treat the resulting code as exhibiting undefined behavior.\n\nFortunately the C standard is pretty clear about this.  The undefined\nbehavior here is at run time, not compile time.  As you suggested in\nan earlier reply, the 'list->items &&' effectively guards the\n'list->items + list->nr' to prevent that undefined behavior.\n\nI'll send a patch with a commit message saying so to try to close out\nthis discussion.\n\nThanks,\nJonathan\n"},{"id":"328439","messageId":"20170920023008.GB126984@aiede.mtv.corp.google.com","threadId":"46757","inReplyTo":"b03c7b09-853f-a2ed-f73e-7d946c90cedb@gmail.com","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-20T02:30:08Z","receivedAt":"2017-09-20T02:30:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nKaartic Sivaraam wrote:\n> On Saturday 16 September 2017 09:36 AM, Michael Haggerty wrote:\n>> Jonathan Nieder wrote:\n\n>>> Does the following alternate fix work?  I think I prefer it because\n>>> it doesn't require introducing a new global. [...]\n>>>   #define for_each_string_list_item(item,list) \\\n>>> -\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n>>> +\tfor (item = (list)->items; \\\n>>> +\t     (list)->items && item < (list)->items + (list)->nr; \\\n>>> +\t     ++item)\n>>\n>> This is the possibility that I was referring to as \"add[ing] overhead to\n>> each iteration of the loop\". I'd rather not add an extra test-and-branch\n>> to every iteration of a loop in which `list->items` is *not* NULL, which\n>> your solution appears to do. Or are compilers routinely able to optimize\n>> the check out?\n>\n> I t seems at least 'gcc' is able to optimize this out even with a -O1\n> and 'clang' optimizes this out with a -O2. Taking a sneak peek at\n> the 'Makefile' shows that our default is -O2.\n>\n> For a proof, see https://godbolt.org/g/CPt73L\n\nFrom that link:\n\n    for ( ;valid_int && *valid_int < 10; (*valid_int)++) {\n        printf(\"Valid instance\");\n    }\n\nBoth gcc and clang are able to optimize out the 'valid_int &&' because\nit is dereferenced on the RHS of the &&.\n\nFor comparison, 'item < (list)->items + (list)->nr' does not\ndereference (list)->items.  So that optimization doesn't apply here.\n\nA smart compiler could be able to take advantage of there being no\nobject pointed to by a null pointer, which means\n\n\titem < (list)->items + (list)->nr\n\nis always false when (list)->items is NULL, which in turn makes a\n'(list)->items &&' test redundant.  But a quick test with gcc 4.8.4\n-O2 finds that at least this compiler does not contain such an\noptimization.  The overhead Michael Haggerty mentioned is real.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"328442","messageId":"xmqqd16mowig.fsf@gitster.mtv.corp.google.com","threadId":"46757","inReplyTo":"20170920023008.GB126984@aiede.mtv.corp.google.com","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-20T03:54:15Z","receivedAt":"2017-09-20T03:54:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> ...  But a quick test with gcc 4.8.4\n> -O2 finds that at least this compiler does not contain such an\n> optimization.  The overhead Michael Haggerty mentioned is real.\n\nStill, I have a feeling that users of string_list wouldn't care \nthe overhead of single pointer NULL-ness check.\n\n - apply.c collects conflicted paths and reports them with fprintf().\n\n - builtin/clean.c uses the function to walk the list of paths to be\n   removed, and either does a human interaction (for \"-i\" codepath)\n   or goes to the filesystem to remove things.\n\n - builtin/config.c uses it in get_urlmatch() in preparation for\n   doing network-y things.\n\n - builtin/describe.c walks the list of exclude and include patterns\n   to run wildmatch on the candidate reference name to filter it out.\n\n ...\n\nIn all of these examples, what happens for each item in the loop\nseems to me far heavier than the overhead this macro adds.\n\nSo...\n\n\n"},{"id":"328448","messageId":"xmqqk20une8p.fsf@gitster.mtv.corp.google.com","threadId":"46757","inReplyTo":"20170920014305.GA126984@aiede.mtv.corp.google.com","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-20T05:14:14Z","receivedAt":"2017-09-20T05:14:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> I'll send a patch with a commit message saying so to try to close out\n> this discussion.\n\nThanks.  One less thing we have to worry about ;-)\n"},{"id":"328449","messageId":"20170920052705.GC126984@aiede.mtv.corp.google.com","threadId":"46757","inReplyTo":"xmqqd16mowig.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-20T05:27:05Z","receivedAt":"2017-09-20T05:27:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\nIf you pass a newly initialized or newly cleared `string_list` to\n`for_each_string_list_item()`, then the latter does\n\n    for (\n            item = (list)->items; /* NULL */\n            item < (list)->items + (list)->nr; /* NULL + 0 */\n            ++item)\n\nEven though this probably works almost everywhere, it is undefined\nbehavior, and it could plausibly cause highly-optimizing compilers to\nmisbehave.  C99 section 6.5.6 paragraph 8 explains:\n\n    If both the pointer operand and the result point to elements\n    of the same array object, or one past the last element of the\n    array object, the evaluation shall not produce an overflow;\n    otherwise, the behavior is undefined.\n\nand (6.3.2.3.3) a null pointer does not point to anything.\n\nGuard the loop with a NULL check to make the intent crystal clear to\neven the most pedantic compiler.  A suitably clever compiler could let\nthe NULL check only run in the first iteration, but regardless, this\noverhead is likely to be dwarfed by the work to be done on each item.\n\nThis problem was noticed by Coverity.\n\n[jn: using a NULL check instead of a placeholder empty list;\n fleshed out the commit message based on mailing list discussion]\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n string-list.h | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\nJunio C Hamano wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> ...  But a quick test with gcc 4.8.4\n>> -O2 finds that at least this compiler does not contain such an\n>> optimization.  The overhead Michael Haggerty mentioned is real.\n>\n> Still, I have a feeling that users of string_list wouldn't care\n> the overhead of single pointer NULL-ness check.\n>\n>  - apply.c collects conflicted paths and reports them with fprintf().\n>\n>  - builtin/clean.c uses the function to walk the list of paths to be\n>    removed, and either does a human interaction (for \"-i\" codepath)\n>    or goes to the filesystem to remove things.\n>\n>  - builtin/config.c uses it in get_urlmatch() in preparation for\n>    doing network-y things.\n>\n>  - builtin/describe.c walks the list of exclude and include patterns\n>    to run wildmatch on the candidate reference name to filter it out.\n>\n>  ...\n>\n> In all of these examples, what happens for each item in the loop\n> seems to me far heavier than the overhead this macro adds.\n\nYes, agreed.  As a small tweak,\n\n   #define for_each_string_list_item(item, list) \\\n\tfor (item = ...; item && ...; ...)\n\nproduces nicer assembly than\n\n   #define for_each_string_list_item(item, list) \\\n\tfor (item = ...; list->items && ...; ...)\n\n(By the way, the potential optimization I described isn't valid: we\nknow that when item == NULL and list->items == NULL, list->nr is\nalways zero, but the compiler has no way to know that.  So it can't\neliminate the NULL test.  For comparison, a suitably smart compiler\nshould be able to eliminate a 'list->nr != 0 &&' guard if 'list'\ndoesn't escape in the loop body.)\n\nRecapping the other proposed fixes:\n\nA. Make it an invariant of string_list that items is never NULL and\n   update string_list_init et al to use an empty array.  This is\n   pretty painless until you notice some other structs that embed\n   string_list without using STRING_LIST_INIT.  Updating all those\n   would be too painful.\n\nB. #define for_each_string_list_item(item, list) \\\n\tif (list->items) \\\n\t\tfor (item = ...; ...; ... )\n\n   This breaks a caller like\n\tif (foo)\n\t\tfor_each_string_list_item(item, list)\n\t\t\t...\n\telse\n\t\t...\n\n   making it a non-starter.\n\nC. As Gábor suggested,\n   #define for_each_string_list_item(item, list) \\\n   \tif (!list->items) \\\n\t\t; /* nothing to do */ \\\n\telse \\\n\t\tfor (item = ...; ...; ...)\n\n   This handles the caller from (B) correctly.  But it produces\n   compiler warnings for a caller like\n\n\tif (foo)\n\t\tfor_each_string_list_item(item, list)\n\t\t\t...\n\n   There is only one instance of that construct in git today.  It\n   looks nicer anyway with braces, so this approach would also be\n   promising.\n\nD. Eliminate for_each_string_list_item and let callers just do\n\n\tunsigned int i;\n\tfor (i = 0; i < list->nr; i++) {\n\t\tstruct string_list_item *item = list->items[i];\n\t\t...\n\t}\n\n   Having to declare item is unnecessarily verbose, decreasing the\n   appeal of this option.  I think I like it anyway, but I wasn't able\n   to convince coccinelle to do it.\n\nE. Use subtraction instead of addition:\n   #define for_each_string_list_item(item, list) \\\n   \tfor (item = ...; \\\n\t     (item == list->items ? 0 : item - list->items) < nr; \\\n\t     item++)\n\n   I expected the compiler to figure out that this is a long way of writing\n   (item - list->items), but at least with gcc 4.8.4 -O2, no such\n   luck.  This generates uglier assembly than the NULL check.\n\ndiff --git a/string-list.h b/string-list.h\nindex 29bfb7ae45..79ae567cbc 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -32,8 +32,10 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n typedef int (*string_list_each_func_t)(struct string_list_item *, void *);\n int for_each_string_list(struct string_list *list,\n \t\t\t string_list_each_func_t, void *cb_data);\n-#define for_each_string_list_item(item,list) \\\n-\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n+#define for_each_string_list_item(item,list)            \\\n+\tfor (item = (list)->items;                      \\\n+\t     item && item < (list)->items + (list)->nr; \\\n+\t     ++item)\n \n /*\n  * Apply want to each item in list, retaining only the ones for which\n-- \n2.14.1.821.g8fa685d3b7\n\n"},{"id":"328451","messageId":"xmqqbmm6nd0v.fsf@gitster.mtv.corp.google.com","threadId":"46757","inReplyTo":"20170920052705.GC126984@aiede.mtv.corp.google.com","subject":"Re: [PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-20T05:40:32Z","receivedAt":"2017-09-20T05:40:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> D. Eliminate for_each_string_list_item and let callers just do\n>\n> \tunsigned int i;\n> \tfor (i = 0; i < list->nr; i++) {\n> \t\tstruct string_list_item *item = list->items[i];\n> \t\t...\n> \t}\n>\n>    Having to declare item is unnecessarily verbose, decreasing the\n>    appeal of this option.  I think I like it anyway, but I wasn't able\n>    to convince coccinelle to do it.\n\nWhen using the macro, item still needs to be declared outside by the\nuser, so it's not all that unpleasant, though.\n\n> E. Use subtraction instead of addition:\n>    #define for_each_string_list_item(item, list) \\\n>    \tfor (item = ...; \\\n> \t     (item == list->items ? 0 : item - list->items) < nr; \\\n> \t     item++)\n>\n>    I expected the compiler to figure out that this is a long way of writing\n>    (item - list->items), but at least with gcc 4.8.4 -O2, no such\n>    luck.  This generates uglier assembly than the NULL check.\n\nYuck.  You cannot easily unsee such an ugliness X-<.\n\nThe patch and explanation above --- looked quite nicely written.\nWill queue.\n\nThanks.\n\n> diff --git a/string-list.h b/string-list.h\n> index 29bfb7ae45..79ae567cbc 100644\n> --- a/string-list.h\n> +++ b/string-list.h\n> @@ -32,8 +32,10 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n>  typedef int (*string_list_each_func_t)(struct string_list_item *, void *);\n>  int for_each_string_list(struct string_list *list,\n>  \t\t\t string_list_each_func_t, void *cb_data);\n> -#define for_each_string_list_item(item,list) \\\n> -\tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n> +#define for_each_string_list_item(item,list)            \\\n> +\tfor (item = (list)->items;                      \\\n> +\t     item && item < (list)->items + (list)->nr; \\\n> +\t     ++item)\n>  \n>  /*\n>   * Apply want to each item in list, retaining only the ones for which\n"},{"id":"328454","messageId":"124b960d-a863-18ef-54a2-b170036dfca2@alum.mit.edu","threadId":"46757","inReplyTo":"20170920052705.GC126984@aiede.mtv.corp.google.com","subject":"Re: [PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-20T07:00:48Z","receivedAt":"2017-09-20T07:00:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/20/2017 07:27 AM, Jonathan Nieder wrote:\n> From: Michael Haggerty <mhagger@alum.mit.edu>\n> \n> If you pass a newly initialized or newly cleared `string_list` to\n> `for_each_string_list_item()`, then the latter does\n> \n>     for (\n>             item = (list)->items; /* NULL */\n>             item < (list)->items + (list)->nr; /* NULL + 0 */\n>             ++item)\n> \n> Even though this probably works almost everywhere, it is undefined\n> behavior, and it could plausibly cause highly-optimizing compilers to\n> misbehave.  C99 section 6.5.6 paragraph 8 explains:\n> \n>     If both the pointer operand and the result point to elements\n>     of the same array object, or one past the last element of the\n>     array object, the evaluation shall not produce an overflow;\n>     otherwise, the behavior is undefined.\n> \n> and (6.3.2.3.3) a null pointer does not point to anything.\n> \n> Guard the loop with a NULL check to make the intent crystal clear to\n> even the most pedantic compiler.  A suitably clever compiler could let\n> the NULL check only run in the first iteration, but regardless, this\n> overhead is likely to be dwarfed by the work to be done on each item.\n> \n> This problem was noticed by Coverity.\n> \n> [jn: using a NULL check instead of a placeholder empty list;\n>  fleshed out the commit message based on mailing list discussion]\n\nThanks for taking this over. This version LGTM.\n\n> [...]\nMichael\n"},{"id":"328455","messageId":"3467b198-8c8c-2bfa-b139-d3ed5ab6b8bc@gmail.com","threadId":"46757","inReplyTo":"20170920023008.GB126984@aiede.mtv.corp.google.com","subject":"Re: [PATCH] for_each_string_list_item(): behave correctly for empty list","fromName":"Kaartic Sivaraam","fromEmail":"kaarticsivaraam91196@gmail.com","sentAt":"2017-09-20T07:35:01Z","receivedAt":"2017-09-20T07:35:15Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"Hi,\n\nThough this thread seems to have reached a conclusion, I just wanted to\nknow what I was missing about the optimisation.\n\nOn Wednesday 20 September 2017 08:00 AM, Jonathan Nieder wrote:\n> From that link:\n>      for ( ;valid_int && *valid_int < 10; (*valid_int)++) {\n>          printf(\"Valid instance\");\n>      }\n>\n> Both gcc and clang are able to optimize out the 'valid_int &&' because\n> it is dereferenced on the RHS of the &&.\n>\n> For comparison, 'item < (list)->items + (list)->nr' does not\n> dereference (list)->items.  So that optimization doesn't apply here.\n>\n> A smart compiler could be able to take advantage of there being no\n> object pointed to by a null pointer, which means\n>\n> \titem < (list)->items + (list)->nr\n>\n> is always false when (list)->items is NULL, which in turn makes a\n> '(list)->items &&' test redundant.  But a quick test with gcc 4.8.4\n> -O2 finds that at least this compiler does not contain such an\n> optimization.  The overhead Michael Haggerty mentioned is real.\n>\n\nI thought the compiler optimized that check out of the loop because the\ncheck was \"invariant\" across loop runs. IOW, the values used in the check\ndidn't change across loop runs so the compiler thought it's better to do\nthe check once outside the loop rather than doing it each time inside\nthe loop. I guess this is some kind of \"loop unswitching\"[1]. I don't \nsee how\ndereferencing influences the optimization here.\n\nJust to be sure, I tried once more to see whether the compiler optimizes \nthis\nor not. This time with a more similar example and even using the macro \nof concern.\nSurprisingly, the compiler did optimize the check out of the loop. This \ntime both\n'gcc' and 'clang' with an -O1 !\n\nhttps://godbolt.org/g/Y6rHc1\nhttps://godbolt.org/g/EMrftw\n\nSo, is the overhead still real or am I missing something?\n\n[1] : https://en.wikipedia.org/wiki/Loop_unswitching\n\n---\nKaartic\n"},{"id":"328456","messageId":"b53ab56f-80e6-588f-fefb-53d7fe22edbd@gmail.com","threadId":"46757","inReplyTo":"20170920052705.GC126984@aiede.mtv.corp.google.com","subject":"Re: [PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list","fromName":"Kaartic Sivaraam","fromEmail":"kaarticsivaraam91196@gmail.com","sentAt":"2017-09-20T07:40:35Z","receivedAt":"2017-09-20T07:40:45Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Wednesday 20 September 2017 10:57 AM, Jonathan Nieder wrote:\n> Guard the loop with a NULL check to make the intent crystal clear to\n> even the most pedantic compiler.  A suitably clever compiler could let\n> the NULL check only run in the first iteration,\n\nNoted this just now. So, the overhead doesn't occur when the compilers\nare clever enough. And as I said in my previous email to this thread, at\nleast 'gcc' and 'clang' seem to be clever enough.\n> ... but regardless, this\n> overhead is likely to be dwarfed by the work to be done on each item.\n\n:-) That's of course seems to be true.\n\n"},{"id":"328464","messageId":"0102015e9f3d2d6b-b68ad740-0847-42e2-beb6-9d3fde4b427f-000000@eu-west-1.amazonses.com","threadId":"46757","inReplyTo":"20170920052705.GC126984@aiede.mtv.corp.google.com","subject":"[PATCH v2] doc: camelCase the config variables to improve readability","fromName":"Kaartic Sivaraam","fromEmail":"kaarticsivaraam91196@gmail.com","sentAt":"2017-09-20T12:22:20Z","receivedAt":"2017-09-20T12:22:26Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"A few configuration variable names of Git are composite words. References\nto such variables in manpages are hard to read because they use all-lowercase\nnames, without indicating where each word ends and begins.\n\nImprove its readability by using camelCase instead.  Git treats these\nnames case-insensitively so this does not affect functionality. This\nalso ensures consistency with other parts of the docs that use camelCase\nfo refer to configuration variable names.\n\nSigned-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n Documentation/git-branch.txt | 4 ++--\n Documentation/git-tag.txt    | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex e292737b9c5ab..58f1e5c9c74e1 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -92,10 +92,10 @@ OPTIONS\n \tall changes made to the branch ref, enabling use of date\n \tbased sha1 expressions such as \"<branchname>@\\{yesterday}\".\n \tNote that in non-bare repositories, reflogs are usually\n-\tenabled by default by the `core.logallrefupdates` config option.\n+\tenabled by default by the `core.logAllRefUpdates` config option.\n \tThe negated form `--no-create-reflog` only overrides an earlier\n \t`--create-reflog`, but currently does not negate the setting of\n-\t`core.logallrefupdates`.\n+\t`core.logAllRefUpdates`.\n \n -f::\n --force::\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 543fb425ee7c1..95e9f391d88fc 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -174,7 +174,7 @@ This option is only applicable when listing tags without annotation lines.\n \t`core.logAllRefUpdates` in linkgit:git-config[1].\n \tThe negated form `--no-create-reflog` only overrides an earlier\n \t`--create-reflog`, but currently does not negate the setting of\n-\t`core.logallrefupdates`.\n+\t`core.logAllRefUpdates`.\n \n <tagname>::\n \tThe name of the tag to create, delete, or describe.\n\n--\nhttps://github.com/git/git/pull/407\n"},{"id":"328470","messageId":"87vakd2v22.fsf@linux-m68k.org","threadId":"46757","inReplyTo":"20170920052705.GC126984@aiede.mtv.corp.google.com","subject":"Re: [PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2017-09-20T16:28:53Z","receivedAt":"2017-09-20T16:29:01Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Sep 19 2017, Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n> B. #define for_each_string_list_item(item, list) \\\n> \tif (list->items) \\\n> \t\tfor (item = ...; ...; ... )\n>\n>    This breaks a caller like\n> \tif (foo)\n> \t\tfor_each_string_list_item(item, list)\n> \t\t\t...\n> \telse\n> \t\t...\n>\n>    making it a non-starter.\n\nThat can be fixed with a dangling else.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"328476","messageId":"20170920173134.GZ27425@aiede.mtv.corp.google.com","threadId":"46757","inReplyTo":"87vakd2v22.fsf@linux-m68k.org","subject":"Re: [PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-20T17:31:34Z","receivedAt":"2017-09-20T17:31:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Andreas Schwab wrote:\n> On Sep 19 2017, Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n>> B. #define for_each_string_list_item(item, list) \\\n>> \tif (list->items) \\\n>> \t\tfor (item = ...; ...; ... )\n>>\n>>    This breaks a caller like\n>> \tif (foo)\n>> \t\tfor_each_string_list_item(item, list)\n>> \t\t\t...\n>> \telse\n>> \t\t...\n>>\n>>    making it a non-starter.\n>\n> That can be fixed with a dangling else.\n\nI believe the fix you're referring to is option C, from the same email\nyou are replying to.  If not, please correct me.\n\nThanks,\nJonathan\n"},{"id":"328506","messageId":"87lgl9rqbq.fsf@linux-m68k.org","threadId":"46757","inReplyTo":"20170920173134.GZ27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2017-09-20T21:51:53Z","receivedAt":"2017-09-20T21:52:02Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Sep 20 2017, Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n> Andreas Schwab wrote:\n>> On Sep 19 2017, Jonathan Nieder <jrnieder@gmail.com> wrote:\n>\n>>> B. #define for_each_string_list_item(item, list) \\\n>>> \tif (list->items) \\\n>>> \t\tfor (item = ...; ...; ... )\n>>>\n>>>    This breaks a caller like\n>>> \tif (foo)\n>>> \t\tfor_each_string_list_item(item, list)\n>>> \t\t\t...\n>>> \telse\n>>> \t\t...\n>>>\n>>>    making it a non-starter.\n>>\n>> That can be fixed with a dangling else.\n>\n> I believe the fix you're referring to is option C, from the same email\n> you are replying to.  If not, please correct me.\n\nA variant thereof, yes.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"328516","messageId":"xmqqtvzwn9bj.fsf@gitster.mtv.corp.google.com","threadId":"46757","inReplyTo":"87lgl9rqbq.fsf@linux-m68k.org","subject":"Re: [PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-21T01:12:48Z","receivedAt":"2017-09-21T01:12:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Schwab <schwab@linux-m68k.org> writes:\n\n> On Sep 20 2017, Jonathan Nieder <jrnieder@gmail.com> wrote:\n>\n>> Andreas Schwab wrote:\n>>> On Sep 19 2017, Jonathan Nieder <jrnieder@gmail.com> wrote:\n>>\n>>>> B. #define for_each_string_list_item(item, list) \\\n>>>> \tif (list->items) \\\n>>>> \t\tfor (item = ...; ...; ... )\n>>>>\n>>>>    This breaks a caller like\n>>>> \tif (foo)\n>>>> \t\tfor_each_string_list_item(item, list)\n>>>> \t\t\t...\n>>>> \telse\n>>>> \t\t...\n>>>>\n>>>>    making it a non-starter.\n>>>\n>>> That can be fixed with a dangling else.\n>>\n>> I believe the fix you're referring to is option C, from the same email\n>> you are replying to.  If not, please correct me.\n>\n> A variant thereof, yes.\n\nNow you make me curious.  How would that variant be different from\noption C. in Jonathan's message?  Perhaps that different version may\nbe a solution to work around the potential issue mentioned in the\ndescription of option C.?\n\n"},{"id":"328571","messageId":"87y3p8uklq.fsf@linux-m68k.org","threadId":"46757","inReplyTo":"xmqqtvzwn9bj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2017-09-21T15:39:29Z","receivedAt":"2017-09-21T15:39:41Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Sep 21 2017, Junio C Hamano <gitster@pobox.com> wrote:\n\n> Now you make me curious.  How would that variant be different from\n> option C. in Jonathan's message?\n\nOnly in the parity of the condition.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"}]}