Re: [PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list
From: Junio C Hamano <hidden>
Date: 2017-09-20 05:40:39
Jonathan Nieder [off-list ref] writes:
D. Eliminate for_each_string_list_item and let callers just do
unsigned int i;
for (i = 0; i < list->nr; i++) {
struct string_list_item *item = list->items[i];
...
}
Having to declare item is unnecessarily verbose, decreasing the
appeal of this option. I think I like it anyway, but I wasn't able
to convince coccinelle to do it.When using the macro, item still needs to be declared outside by the user, so it's not all that unpleasant, though.
E. Use subtraction instead of addition: #define for_each_string_list_item(item, list) \ for (item = ...; \ (item == list->items ? 0 : item - list->items) < nr; \ item++) I expected the compiler to figure out that this is a long way of writing (item - list->items), but at least with gcc 4.8.4 -O2, no such luck. This generates uglier assembly than the NULL check.
Yuck. You cannot easily unsee such an ugliness X-<. The patch and explanation above --- looked quite nicely written. Will queue. Thanks.
quoted hunk
diff --git a/string-list.h b/string-list.h index 29bfb7ae45..79ae567cbc 100644 --- a/string-list.h +++ b/string-list.h@@ -32,8 +32,10 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c typedef int (*string_list_each_func_t)(struct string_list_item *, void *); int for_each_string_list(struct string_list *list, string_list_each_func_t, void *cb_data); -#define for_each_string_list_item(item,list) \ - for (item = (list)->items; item < (list)->items + (list)->nr; ++item) +#define for_each_string_list_item(item,list) \ + for (item = (list)->items; \ + item && item < (list)->items + (list)->nr; \ + ++item) /* * Apply want to each item in list, retaining only the ones for which