Re: [PATCH 02/11] string-list.h: add string_list_{pop, last} functions

Stefan Beller <sbeller@google.com> writes:

verb missing. "We can use a string list as ..., but should we?" is readable. "A string list can be ..., but should it be?" also is.

Instead of all of the above, how about being more direct, i.e. e.g.

	Add a few functions to allow a string-list to be used as a
	stack:

	- string_list_last() lets a caller peek the string_list_item
          at the end of the string list.  The caller needs to be
          aware that it is borrowing a pointer, which can become
          invalid if/when the string_list is resized.

	- string_list_pop() removes the string_list_item at the end
          of the string list.

	You can use them in this pattern:

		while (list.nr) {
			struct string_list_item *item = string_list_last(&list);

			work_on(item);
			string_list_pop(&list);
		}

Then the readers would immediately have enough information to judge if the new API is useful without blindly trusting what you promise.

Two things to note in the above suggested rewrite are:

 - It is curious that _pop() is not accompanied by _push().  It
   probably is a good idea to add the third bullet point that says
   "existing string_list_append() can be used in place of the
   missing string_list_push()."

 - As it already lays out the memory ownership issue, it probably
   makes the description we see below unnecessary (besides, the code
   snippet in there were buggy in at least two counts).

Conceptually, this allows string_list_clear() to be implemented in terms of this function, i.e.

	string_list_clear(struct string_list *list, int free_util)
	{
		while (list->nr)
			string_list_pop(list, free_util);
		free(list->items);
		list->items = NULL;
		list->nr = list->alloc = 0;
	}

It is unfortunate that string_list has become such a kitchen-sink in that a string list whose util field is used in such a way that string_list_clear_func() needs to be used to clear the list cannot be used as a stack. Ideally each of these "features" (like "sorted/unsorted", "owning/borrowing util", "owning/borrowing string", etc.) ought to be orthogonal, but they are not.

inline functions declared in a header file must be marked "static inline", I think.