Re: [PATCH 02/10] string-list.h: add string_list_pop function.

From: Stefan Beller <sbeller@google.com>
Date: Aug 9, 2018, 21:52 UTC
Message-ID: <CAGZ79kZK=5gftetj3XLTbu-n3WKwRUQQDj12yxDBV0WEN8gg9Q@mail.gmail.com>
In-Reply-To: <20180809214148.GD11342@sigill.intra.peff.net>

On Thu, Aug 9, 2018 at 2:41 PM Jeff King <peff@peff.net> wrote:

Show 14 quoted lines

> >
> >       while (list->nr)
> >               work_on(list_pop(list));
> >
> > which is not so bad.
>
> In many cases you can just do:
>
>   while (list->nr) {
>         work_on(list->items[list->nr - 1]);
>         list_remove(list, list->nr - 1);
>   }
>
> and then all of those memory ownership issues like:

[...]

>
> just go away. :)

The only complication here is the lack of list_remove(index), we do have list_remove(string), which internally searches the item and removes it. Hence I did not want to use it.

Another idea I had was to keep the list immutable (except amending, just like a constitution ;-) and store an index of how far we got in that list already. That wastes memory for keeping entries around, but is safe for memory due to its nature.

Show 6 quoted lines

> Where that falls down is if you really need work_on() to put more items
> on the stack, but only after you've removed the current top. But then
> writing it out may still be nicer, because it makes it clear you have to
> do:
>
>   const char *cur_string = xstrdup(list->items[list->nr-1].string);

Another way would be to use

  string_list_pop(&list, &string_dst, &util_dst);
i.e.
  /* Returns 0 if the dst was filled */
  int (struct string_list *, char **, void**)

as then we do not expose the internals and would not have issues with reallocs.

> if you want the data to live past the removal.

In the code proposed there are no additions (hence no reallocs) and the need for the data is short lived.

But I can see how the design was just fitting my purpose and we could come up with some better API.

Thanks, Stefan