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

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Tue, Sep 19, 2017 at 8:51 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 09/19/2017 02:08 AM, Stefan Beller wrote:
>>> I am hoping that this last one is not allowed and we can use the
>>> "same condition is checked every time we loop" version that hides
>>> the uglyness inside the macro.
>>
>> By which you are referring to Jonathans solution posted.
>> Maybe we can combine the two solutions (checking for thelist
>> to not be NULL once, by Jonathan) and using an outer structure
>> (SZEDERs solution) by replacing the condition by a for loop,
>> roughly (untested):
>>
>> #define for_each_string_list_item(item,list) \
>> -       for (item = (list)->items; item < (list)->items + (list)->nr; ++item)
>> +    for (; list; list = NULL)
>> +        for (item = (list)->items; item < (list)->items + (list)->nr; ++item)
>>
>> as that would not mingle with any dangling else clause.
>> It is also just one statement, such that
>>
>>     if (bla)
>>       for_each_string_list_item {
>>         baz(item);
>>       }
>>     else
>>       foo;
>>
>> still works.
>>
>> Are there downsides to this combined approach?
>
> On the plus side, it's pleasantly devious; I wouldn't have thought of
> using a `for` loop for the initial test. But it doesn't work as written,
> because (1) we don't need to guard against `list` being NULL, but rather
> `list->items`; and (2) we don't have the liberty to set `list = NULL`
> (or `list->items = NULL`, because `list` is owned by the caller and we
> shouldn't modify it.
>
> The following is a bit closer:
>
> #define for_each_string_list_item(item,list) \
>         for (item = (list)->items; item; item = NULL) \
>                 for (; item < (list)->items + (list)->nr; ++item)
>
> But I think that also fails, because a callsite that does
>
>         for_each_string_list_item(myitem, mylist)
>                 if (myitem.util)
>                         break;
>
> would expect that `myitem` is still set after breaking out of the loop,
> whereas the outer `for` loop would reset it to NULL.
>
> If `break` were an expression we could do something like
>
> #define for_each_string_list_item(item,list) \
>         for (item = (list)->items; item; break) \
>                 for (; item < (list)->items + (list)->nr; ++item)

A bit "futuristic" option along these lines could be something like
this, using a scoped loop variable in the outer loop to ensure that
it's executed at most once:

  #define for_each_string_list_item(item,list) \
      for (int f_e_s_l_i = 1; (list)->items && f_e_s_l_i; f_e_s_l_i = 0) \
          for (item = (list)->items; item < (list)->items + (list)->nr; ++item)

The high number of underscores are an attempt to make reasonably sure
that the macro's loop variable doesn't shadow any variable in its
callers or isn't being shadowed in the loop body, which might(?)
trigger warnings in some compilers.

Alas we don't allow scoping the loop variable in for loops, and even a
test balloon patch didn't make it into git.git.

  https://public-inbox.org/git/20170719181956.15845-1-sbeller@google.com/T/#u


> So I think we're still left with the suggestions of Jonathan or Gábor.
> Or the bigger change of initializing `string_list::items` to point at an
> empty sentinal array (similar to `strbuf_slopbuf`) rather than NULL.
> Personally, I think that Jonathan's approach makes the most sense,
> unless somebody wants to jump in an implement a `string_list_slopbuf`.
>
> By the way, I wonder if any open-coded loops over `string_lists` make
> the same mistake as the macro?