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

From

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

Date

Sep 16, 2017, 11:51 UTC

Message-ID

<20170916115118.15490-1-szeder.dev@gmail.com>

In-Reply-To

<b8951886-feab-a87a-9683-3c155cfa98a8@alum.mit.edu>

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

diff --git a/string-list.h b/string-list.h
index 29bfb7ae4..9eed47de0 100644
--- a/string-list.h
+++ b/string-list.h
@@ -32,8 +32,11 @@ 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) 	\
+	if ((list)->items == NULL) {		\
+		/* empty list, do nothing */	\
+	} else					\
+		for (item = (list)->items; item < (list)->items + (list)->nr; ++item)
 
 /*
  * Apply want to each item in list, retaining only the ones for which

This way there would be neither additional overhead in each iteration
nor a new global.

Alas, there is a catch.  We can't use curly braces in the macro's else
branch, because the macro would contain only the opening brace but not
the closing one, which must come after the end of the loop's body.
This means that the modified macro couldn't be used in if-else
branches which themselves don't have curly braces, because it causes
ambiguity:

  if (condition)
      for_each_string_list_item(item, list)
          a_simple_oneliner(item);

Our coding guidelines encourage this style for one-liner loop bodies,
and there is indeed one such place in our codebase, so the following
hunk is needed as well:

diff --git a/send-pack.c b/send-pack.c
index 11d6f3d98..00fa1622f 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -295,9 +295,10 @@ static int generate_push_cert(struct strbuf *req_buf,
 	}
 	if (push_cert_nonce[0])
 		strbuf_addf(&cert, "nonce %s\n", push_cert_nonce);
-	if (args->push_options)
+	if (args->push_options) {
 		for_each_string_list_item(item, args->push_options)
 			strbuf_addf(&cert, "push-option %s\n", item->string);
+	}
 	strbuf_addstr(&cert, "\n");
 
 	for (ref = remote_refs; ref; ref = ref->next) {


Luckily, reasonably modern compilers warn about such ambiguity, so
perhaps this is an acceptable compromise?


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