Re: [PATCH v5 05/11] string-list: add string_list_remove function

On Tue, Apr 18, 2017 at 4:36 PM, Brandon Williams [off-list ref] wrote:

On 04/18, Stefan Beller wrote:

quoted

On Tue, Apr 18, 2017 at 4:17 PM, Brandon Williams [off-list ref] wrote:

quoted

+void string_list_remove(struct string_list *list, const char *string,
+                       int free_util)
+{
+       int exact_match;
+       int i = get_entry_index(list, string, &exact_match);
+
+       if (exact_match) {
+               if (list->strdup_strings)
+                       free(list->items[i].string);
+               if (free_util)
+                       free(list->items[i].util);
+
+               list->nr--;
+               memmove(list->items + i, list->items + i + 1,
+                       (list->nr - i) * sizeof(struct string_list_item));
+       }

Looks correct. I shortly wondered if we'd have any value in returing
`exact_match`, as that may save the caller some code, as I imagine the
caller to be:

  if (!string_list_has_string(&list, string))
    die("BUG: ...");
  string_list_remove(&list, string, 0);

which could be simplified if we had the exact_match returned, i.e.
the string_list_remove returns the implicit string_list_has_string.

I don't really see the value in this, as the only caller doesn't need it
right now.

yeah, I guess we can add such functionality later.

quoted

quoted

 /*
+ * Removes the given string from the sorted list.

What happens when the string is not found?

nothing.

yeah that is what I figured from reading the code. The question could
have been worded: Do we want to document what happens if the string
is not found? (A reader in the future may wonder if it is even allowed to
call this function without having consulted string_list_has_string).

quoted

quoted

+ */
+void string_list_remove(struct string_list *list, const char *string, int free_util);

How much do we care about (eventual) consistency? ;)
i.e. mark it extern ?

If I need to do another reroll I can mark it extern.

Thanks! This doesn't require a reroll on its own and all other patches
look sane to me.

Thanks,
Stefan