[1/9] string-list: add string_list_{pop, last} functions
diff mbox series

Message ID 20180911234951.14129-2-sbeller@google.com
State New
Headers show
Series
  • fetch: make sure submodule oids are fetched
Related show

Commit Message

Stefan Beller Sept. 11, 2018, 11:49 p.m. UTC
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.

 - _pop usually has a friend _push. This role is taken by
    string_list_append already, as they are not symmetrical
    in our code base: _append returns the pointer, such that
    adding a util is easy, but _pop doesn't return such a pointer.

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

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 string-list.c | 14 ++++++++++++++
 string-list.h | 11 +++++++++++
 2 files changed, 25 insertions(+)

Comments

Ramsay Jones Sept. 12, 2018, 2:24 a.m. UTC | #1
On 12/09/18 00:49, Stefan Beller wrote:
> 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.
> 
>  - _pop usually has a friend _push. This role is taken by
>     string_list_append already, as they are not symmetrical
>     in our code base: _append returns the pointer, such that
>     adding a util is easy, but _pop doesn't return such a pointer.
> 
> 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);

string_list_pop() takes a second int parameter (free_util).

ATB,
Ramsay Jones
Junio C Hamano Sept. 12, 2018, 5:52 p.m. UTC | #2
Stefan Beller <sbeller@google.com> writes:

> 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.
>
>  - _pop usually has a friend _push. This role is taken by
>     string_list_append already, as they are not symmetrical
>     in our code base: _append returns the pointer, such that
>     adding a util is easy, but _pop doesn't return such a pointer.

This third-item somehow is indented one place too deeply.

"our pop() does not return a pointer, so we won't introduce push()
that does return a pointer"?  Am I reading the third item right?

pop() and push() usually go together, but they are opposite
operations, they are expected to behave oppositely, and they are
expected to be interfaced differently.  pop() that does not return a
pointer is no better or no worse match to push() that returns a
pointer, no?

Lack of something_push(), when something_pop() exists, would always
be surprising to anybody new to the "something" API, and no amount
of mumbling would justify it, I would think, but this third item
sounds like it is trying to make a lame excuse when there is no need
to.

Wouldn't it be sufficient to say "there is no string_list_push();
string_list_append() can be used in its place" and stop there?

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

"free_util" as the second argument to this sample call is missing.

>  string-list.c | 14 ++++++++++++++
>  string-list.h | 11 +++++++++++
>  2 files changed, 25 insertions(+)

> diff --git a/string-list.c b/string-list.c
> index 771c4550980..04db2b537c0 100644
> --- a/string-list.c
> +++ b/string-list.c
> @@ -80,6 +80,20 @@ void string_list_remove(struct string_list *list, const char *string,
>  	}
>  }
>  
> +void string_list_pop(struct string_list *list, int free_util)
> +{
> +	if (list->nr == 0)
> +		BUG("tried to remove an item from empty string list");
> +
> +	if (list->strdup_strings)
> +		free(list->items[list->nr - 1].string);
> +
> +	if (free_util)
> +		free(list->items[list->nr - 1].util);
> +
> +	list->nr--;
> +}
> +
>  int string_list_has_string(const struct string_list *list, const char *string)
>  {
>  	int exact_match;
> diff --git a/string-list.h b/string-list.h
> index ff8f6094a33..ce2528bbe15 100644
> --- a/string-list.h
> +++ b/string-list.h
> @@ -191,6 +191,17 @@ extern void string_list_remove(struct string_list *list, const char *string,
>   */
>  struct string_list_item *string_list_lookup(struct string_list *list, const char *string);
>  
> +/**
> + * Removes the last item from the list.
> + * The caller must ensure that the list is not empty.
> + */
> +void string_list_pop(struct string_list *list, int free_util);
> +
> +static inline struct string_list_item *string_list_last(struct string_list *list)

We may want to warn users that pop(), append(), etc. shouldn't be
done while using the returned value from this function in an in-code
comment or docstring.

> +{
> +	return &list->items[list->nr - 1];
> +}
> +

Other than that, nicely done.

>  /*
>   * Remove all but the first of consecutive entries with the same
>   * string value.  If free_util is true, call free() on the util

Patch
diff mbox series

diff --git a/string-list.c b/string-list.c
index 771c4550980..04db2b537c0 100644
--- a/string-list.c
+++ b/string-list.c
@@ -80,6 +80,20 @@  void string_list_remove(struct string_list *list, const char *string,
 	}
 }
 
+void string_list_pop(struct string_list *list, int free_util)
+{
+	if (list->nr == 0)
+		BUG("tried to remove an item from empty string list");
+
+	if (list->strdup_strings)
+		free(list->items[list->nr - 1].string);
+
+	if (free_util)
+		free(list->items[list->nr - 1].util);
+
+	list->nr--;
+}
+
 int string_list_has_string(const struct string_list *list, const char *string)
 {
 	int exact_match;
diff --git a/string-list.h b/string-list.h
index ff8f6094a33..ce2528bbe15 100644
--- a/string-list.h
+++ b/string-list.h
@@ -191,6 +191,17 @@  extern void string_list_remove(struct string_list *list, const char *string,
  */
 struct string_list_item *string_list_lookup(struct string_list *list, const char *string);
 
+/**
+ * Removes the last item from the list.
+ * The caller must ensure that the list is not empty.
+ */
+void string_list_pop(struct string_list *list, int free_util);
+
+static inline struct string_list_item *string_list_last(struct string_list *list)
+{
+	return &list->items[list->nr - 1];
+}
+
 /*
  * Remove all but the first of consecutive entries with the same
  * string value.  If free_util is true, call free() on the util