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

From: Stefan Beller <sbeller@google.com>
Date: Aug 9, 2018, 22:10 UTC
Message-ID: <CAGZ79kaDSkwUqfbtRGtfuLKMQs+JcfA75T2cHNxK52hYP+cF-w@mail.gmail.com>
In-Reply-To: <20180809215637.GA12441@sigill.intra.peff.net>

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

Show 10 quoted lines

>
> I think you could have helpers to spell the two lines above even more
> nicely:
>
>   while (list->nr) {
>         work_on(list_top(list));
>         list_pop(list); /* note this doesn't return anything! */
>   }
>
> But yes, it's not possible with the current functions.

I like this one most, and as we manage our own memory via the alloc variable we also would not see a lot of memory churn for constant push/pop traffic.

> You can also use a list.h linked-list. Then removal from the list and
> freeing are two separate operations (but it exercises your malloc a lot
> more if you're constantly pushing and popping).

For that I'd have to define my own type derived from list.h to carry the string and the util pointer, which looks very similar to the string_list we already have from a users POV.

Show 20 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.
>
> Yes, I almost suggested that, but there's the question of memory
> ownership of string_dst. Does it need freed or not? Is that answer
> dependent on the strdup_strings flag?

Sure. But as the caller, you should know? You constructed that string_list.