Re: [PATCH v3 01/11] argv_array: offer to split a string by whitespace

From: Stefan Beller <sbeller@google.com>
Date: Apr 20, 2018, 23:29 UTC
Message-ID: <CAGZ79kaH9RWJ=bK=6tEOt2nXjXopC=uxasH6NUUASieRHL7giA@mail.gmail.com>
In-Reply-To: <8171fd3920846e37c9f1cab6f1ac658dbfb01bfe.1524262793.git.johannes.schindelin@gmx.de>

On Fri, Apr 20, 2018 at 3:20 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 40 quoted lines

> This is a simple function that will interpret a string as a whitespace
> delimited list of values, and add those values into the array.
>
> Note: this function does not (yet) offer to split by arbitrary delimiters,
> or keep empty values in case of runs of whitespace, or de-quote Unix shell
> style. All fo this functionality can be added later, when and if needed.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  argv-array.c | 20 ++++++++++++++++++++
>  argv-array.h |  1 +
>  2 files changed, 21 insertions(+)
>
> diff --git a/argv-array.c b/argv-array.c
> index 5d370fa3366..cb5bcd2c064 100644
> --- a/argv-array.c
> +++ b/argv-array.c
> @@ -64,6 +64,26 @@ void argv_array_pop(struct argv_array *array)
>         array->argc--;
>  }
>
> +void argv_array_split(struct argv_array *array, const char *to_split)
> +{
> +       while (isspace(*to_split))
> +               to_split++;
> +       for (;;) {
> +               const char *p = to_split;
> +
> +               if (!*p)
> +                       break;
> +
> +               while (*p && !isspace(*p))
> +                       p++;
> +               argv_array_push_nodup(array, xstrndup(to_split, p - to_split));
> +
> +               while (isspace(*p))
> +                       p++;
> +               to_split = p;
> +       }
> +}

The code looks correct to me.

Though this seems so low level, that I find it hard to accept to implement yet another low level split function. Would reuse of strbuf_split or string_list_split make sense?

Looking at the user in patch 5 you really want to have the argv array, though, so it cannot be pushed to an even higher abstraction layer and be solved there. You really want a string -> argv array split, which would mean we'd have to do the split via string -> {strbufs, stringlist} and then perform a conversion from that to argv array and both conversions look ugly as we'd need to iterate their specific data structure and push each element itself again.

So I guess we rather implement it yet another time.

Looking at their function declarations:

/*
 * lots of very good comments for string list splitting
 */
int string_list_split(struct string_list *list, const char *string,
          int delim, int maxsplit);

/*
 * lots of very good comments for strbuf splitting
 */
static inline struct strbuf **strbuf_split(const struct strbuf *sb,
           int terminator)

I find they have comments in common as well as the terminator. In the commit message you defer the implementation of a terminating symbol, as we can do it later. Also the isspace takes more than one delimiter, which the others do not.

I am debating myself if I want to ask for a comment, as argv-array.h is very short for now and it would be consistent not to add a comment.

Thanks, Stefan