Re: [PATCH v2 09/27] strvec: introduce new `strvec_splice()` function
- From
Toon Claes <toon@iotcl.com>
- Date
- Nov 20, 2024, 08:37 UTC
- Message-ID
- <877c8yti5n.fsf@iotcl.com>
- In-Reply-To
- <20241111-b4-pks-leak-fixes-pt10-v2-9-6154bf91f0b0@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 24 quoted lines
> 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 <ps@pks.im>
> ---
> 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); }
Show 26 quoted lines
> + 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?
Show 6 quoted lines
> + * 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