Re: [PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 20, 2017, 05:40 UTC
Message-ID: <xmqqbmm6nd0v.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170920052705.GC126984@aiede.mtv.corp.google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 11 quoted lines

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

Show 9 quoted lines

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

Show 17 quoted lines

> 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