Re: [PATCH] for_each_string_list_item(): behave correctly for empty list

[Michael Haggerty <mhagger@alum.mit.edu>]

On 09/15/2017 08:43 PM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> If you pass a newly-initialized or newly-cleared `string_list` to
>> `for_each_string_list_item()`, then the latter does
>>
>>     for (
>>             item = (list)->items; /* note, this is NULL */
>>             item < (list)->items + (list)->nr; /* note: NULL + 0 */
>>             ++item)
>>
>> Even though this probably works almost everywhere, it is undefined
>> behavior, and it could plausibly cause highly-optimizing compilers to
>> misbehave.
> 
> Wait, NULL + 0 is undefined behavior?
> 
> *checks the standard*  [...]
> NULL doesn't point to anything so it looks like adding 0 to a null
> pointer is indeed undefined.

Thanks for the legal work :-)

>                             (As a piece of trivia, strictly speaking
> NULL + 0 would be undefined on some implementations and defined on
> others, since an implementation is permitted to #define NULL to 0.)

Isn't that the very definition of "undefined behavior", in the sense of
a language standard?

> [...]
>> It would be a pain to have to change the signature of this macro, and
>> we'd prefer not to add overhead to each iteration of the loop. So
>> instead, whenever `list->items` is NULL, initialize `item` to point at
>> a dummy `string_list_item` created for the purpose.
> 
> What signature change do you mean?  I don't understand what this
> paragraph is alluding to.

I was thinking that one solution would be for the caller to provide a
`size_t` variable for the macro's use as a counter (since I don't see a
way for the macro to declare its own counter). The options are pretty
limited because whatever the macro expands to has to play the same
syntactic role as `for (...; ...; ...)`.

> [...]
> Does the following alternate fix work?  I think I prefer it because
> it doesn't require introducing a new global. [...]
>  #define for_each_string_list_item(item,list) \
> -	for (item = (list)->items; item < (list)->items + (list)->nr; ++item)
> +	for (item = (list)->items; \
> +	     (list)->items && item < (list)->items + (list)->nr; \
> +	     ++item)

This is the possibility that I was referring to as "add[ing] overhead to
each iteration of the loop". I'd rather not add an extra test-and-branch
to every iteration of a loop in which `list->items` is *not* NULL, which
your solution appears to do. Or are compilers routinely able to optimize
the check out?

The new global might be aesthetically unpleasant, but it only costs two
words of memory, so I don't see it as a big disadvantage.

Another, more invasive change would be to initialize
`string_list::items` to *always* point at `dummy_string_list_item`,
rather similar to how `strbuf_slopbuf` is pointed at by empty `strbuf`s.
But I really don't think the effort would be justified.

Michael