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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Sep 16, 2017, 04:06 UTC
Message-ID: <b8951886-feab-a87a-9683-3c155cfa98a8@alum.mit.edu>
In-Reply-To: <20170915184323.GU27425@aiede.mtv.corp.google.com>

On 09/15/2017 08:43 PM, Jonathan Nieder wrote:

Show 19 quoted lines

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

Show 8 quoted lines

> [...]
>> 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 (...; ...; ...)`.

Show 8 quoted lines

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