Subject: Re: [PATCH v2 09/27] strvec: introduce new `strvec_splice()` function

From: Toon Claes <[email-masked]>
Date: Wed, 20 Nov 2024 09:37:40 +0100

Patrick Steinhardt <[email-masked]> writes:

> Introduce a new `strvec_splice()` function that can replace a range of
> strings in the vector with another array of strings. This function will
> be used in subsequent commits.
>
> Signed-off-by: Patrick Steinhardt <[email-masked]>
> ---
>  strvec.c              | 19 +++++++++++++++
>  strvec.h              |  9 +++++++
>  t/unit-tests/strvec.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 93 insertions(+)
>
> diff --git a/strvec.c b/strvec.c
> index f712070f5745d5f998d0846ac4009441dddfa500..81075c50cca4fe44608775541d876294a79d9e4e 100644
> --- a/strvec.c
> +++ b/strvec.c
> @@ -56,6 +56,25 @@ void strvec_pushv(struct strvec *array, const char **items)
>  		strvec_push(array, *items);
>  }
>  
> +void strvec_splice(struct strvec *array, size_t pos, size_t len,
> +		   const char **replacement, size_t replacement_len)
> +{
> +	if (pos + len > array->alloc)
> +		BUG("range outside of array boundary");

Why aren't you checking against array->nr? I was trying a test case for
this, and this seems to be unexpected behavior:

	void test_strvec__splice_insert_after_nr(void)
	{
		struct strvec vec = STRVEC_INIT;
		const char *replacement[] = { "1" };

		strvec_pushl(&vec, "foo", "bar", "baz", "buzz", "fuzz", NULL);
		strvec_pop(&vec);
		check_strvec(&vec, "foo", "bar", "baz", "buzz", NULL);
		strvec_pop(&vec);
		check_strvec(&vec, "foo", "bar", "baz", NULL);
		strvec_pop(&vec);
		strvec_splice(&vec, 4, 1, replacement, ARRAY_SIZE(replacement));
		check_strvec(&vec, "foo", "bar", "baz", NULL, "1", NULL);
		strvec_clear(&vec);
	}

> +	if (replacement_len > len)
> +		ALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,
> +			   array->alloc);
> +	for (size_t i = 0; i < len; i++)
> +		free((char *)array->v[pos + i]);
> +	if (replacement_len != len) {
> +		memmove(array->v + pos + replacement_len, array->v + pos + len,
> +			(array->nr - pos - len + 1) * sizeof(char *));
> +		array->nr += (replacement_len - len);
> +	}
> +	for (size_t i = 0; i < replacement_len; i++)
> +		array->v[pos + i] = xstrdup(replacement[i]);
> +}
> +
>  const char *strvec_replace(struct strvec *array, size_t idx, const char *replacement)
>  {
>  	char *to_free;
> diff --git a/strvec.h b/strvec.h
> index 4b73c1f092e9b016ce3299035477713c6267cdae..4e61cc9336938a95318974903f9b35dcdc4da1cd 100644
> --- a/strvec.h
> +++ b/strvec.h
> @@ -67,6 +67,15 @@ void strvec_pushl(struct strvec *, ...);
>  /* Push a null-terminated array of strings onto the end of the array. */
>  void strvec_pushv(struct strvec *, const char **);
>  
> +/*

Tiniest nit: I see the majority of the function comments in this file
start with a double asterisk, should we do the same here?

> + * Replace `len` values starting at `pos` with the provided replacement
> + * strings. If `len` is zero this is effectively an insert at the given `pos`.
> + * If `replacement_len` is zero this is effectively a delete of `len` items
> + * starting at `pos`.
> + */
> +void strvec_splice(struct strvec *array, size_t pos, size_t len,

In this file we seem to commonly use `idx` instead of `pos`.

-- 
Toon