On Fri, Apr 20, 2018 at 3:20 PM, Johannes Schindelin
<johannes.schindelin@xxxxxx> wrote:
> 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@xxxxxx>
> ---
> 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
