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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Sep 19, 2017, 06:51 UTC
Message-ID: <dab2d555-7e09-4eb3-19b8-cab085626bbe@alum.mit.edu>
In-Reply-To: <CAGZ79kYXDhcVXd2C-x6e=o7jYdKqV22DY45c7E2TeuhKLfn26w@mail.gmail.com>

On 09/19/2017 02:08 AM, Stefan Beller wrote:

Show 28 quoted lines

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

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?

Michael