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

[Patrick Steinhardt <ps@pks.im>]

On Wed, Nov 20, 2024 at 09:37:40AM +0100, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> 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 <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:

Oh, good catch!

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

I'd love to add such a test and verify that it fails as expected. But
the problem is that the API we have just `BUG()`s and thus causes the
program to die. We could adapt the new function to not die but instead
bubble up error codes, but I'd rather do that in a separate patch series
that goes over the whole interface.

> > 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?

Double asterisks are typically used in contexts where comments should be
extracted via tools like Doxygen. We don't do that in Git, so I don't
see a reason to have the double asterisk. Our CodingGuidelines don't
mention double asterisks, either.

> > + * 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`.

Fair, will adapt.

Patrick