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

From: Brandon Williams <bmwill@google.com>
Date: Apr 18, 2017, 23:36 UTC
Message-ID: <20170418233637.GB41018@google.com>
In-Reply-To: <CAGZ79kai79=yZc5ywmG_gYcPnva7C8zVY7oGVqHR39TTyq_O-w@mail.gmail.com>

On 04/18, Stefan Beller wrote:

Show 30 quoted lines

> On Tue, Apr 18, 2017 at 4:17 PM, Brandon Williams <bmwill@google.com> wrote:
> 
> >
> > +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.

Show 5 quoted lines

> 
> >  /*
> > + * Removes the given string from the sorted list.
> 
> What happens when the string is not found?

nothing.

Show 6 quoted lines

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

-- 
Brandon Williams