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

From: Jeff King <peff@peff.net>
Date: Aug 9, 2018, 21:41 UTC
Message-ID: <20180809214148.GD11342@sigill.intra.peff.net>
In-Reply-To: <xmqqzhxv9qkj.fsf@gitster-ct.c.googlers.com>

On Thu, Aug 09, 2018 at 02:29:32PM -0700, Junio C Hamano wrote:

Show 22 quoted lines

> >> +struct string_list_item *string_list_pop(struct string_list *list)
> >> +{
> >> +       if (list->nr == 0)
> >> +               return 0;
> >
> > return NULL, not 0.
> 
> It is OK to return NULL, which may make the caller a bit simpler,
> i.e.
> 
> 	while (item = list_pop(list))
> 		work_on(item);
> 
> but if we consider it a good discipline for the caller to see if
> there are still things on the stack before attempting to pop, then
> instead of returning NULL, we can have BUG("") there, which requires
> the caller to be more like this:
> 
> 	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:

Show 11 quoted lines

> > The memory ownership is now with the caller. That is, the caller needs
> > to check/know `list->strdup_strings` and know `free_util` to be able to
> > properly free all memory.
> 
> > OTOH, the pointer returned by this function is only guaranteed to be
> > valid until you start inserting into the list (well, you can do one
> > insertion per pop without worrying, but that's quite detailed
> > implementation knowledge).
> 
> Yes, it is a more grave limitation when using string_list as a
> stack.  A single realloc() and you are dead X-<.

just go away. :)

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);

if you want the data to live past the removal.

-Peff