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

From: Stefan Beller <sbeller@google.com>
Date: Apr 18, 2017, 23:31 UTC
Message-ID: <CAGZ79kai79=yZc5ywmG_gYcPnva7C8zVY7oGVqHR39TTyq_O-w@mail.gmail.com>
In-Reply-To: <20170418231805.61835-6-bmwill@google.com>

On Tue, Apr 18, 2017 at 4:17 PM, Brandon Williams <bmwill@google.com> wrote:

Show 17 quoted lines

>
> +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.

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

What happens when the string is not found?

> + */
> +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 ?

Thanks, Stefan