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

[Junio C Hamano <gitster@pobox.com>]

Martin Ågren <martin.agren@gmail.com> writes:

> On 9 August 2018 at 00:17, Stefan Beller <sbeller@google.com> wrote:
>> A string list can be used as a stack, but should we? A later patch shows
>> how useful this will be.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  string-list.c | 8 ++++++++
>>  string-list.h | 6 ++++++
>>  2 files changed, 14 insertions(+)
>>
>> diff --git a/string-list.c b/string-list.c
>> index 9f651bb4294..ea80afc8a0c 100644
>> --- a/string-list.c
>> +++ b/string-list.c
>> @@ -80,6 +80,14 @@ void string_list_remove(struct string_list *list, const char *string,
>>         }
>>  }
>>
>> +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.

>> +/**
>> + * Returns the last item inserted and removes it from the list.
>> + * If the list is empty, return NULL.
>> + */
>> +struct string_list_item *string_list_pop(struct string_list *list);
>> +
>
> 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-<.