[PATCH v2] strvec: `strvec_splice()` to a statically initialized vector

From

Rubén Justo <rjusto@gmail.com>

Date

Dec 3, 2024, 19:47 UTC

Message-ID

<5bea9f20-eb0d-409d-8f37-f20697d6ce14@gmail.com>

In-Reply-To

<37d0abbf-c703-481d-9f26-b237aac54c05@gmail.com>

    As a side note, this is the current `strvec_pop()`:

    void strvec_pop(struct strvec *array)
    {
    	if (!array->nr)
    		return;
    	free((char *)array->v[array->nr - 1]);
    	array->v[array->nr - 1] = NULL;
    	array->nr--;
    }

    So, with `strvec_pop()` an instance can become empty but it does
    not going to be the an "empty-singleton".

       struct strvec arr = STRVEC_INIT;
       int a = 0, b = 0;

       ... no modification to arr, a or b ...

       const char *rep[] = { "foo" };
       strvec_splice(&arr, a, b, rep, ARRAY_SIZE(rep));

So, we'll try to add elements to an "empty-singleton" strvec instance.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

Junio: My message in the previous iteration was aimed at readers like
Patrick, who is also the author of `strvec_splice()`.  I certainly
assumed too much prior knowledge, which made the review unnecessarily
laborious.

Thanks.

 strvec.c              | 10 ++++++----
 t/unit-tests/strvec.c | 10 ++++++++++
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/strvec.c b/strvec.c
index d1cf4e2496..087c020f5b 100644
--- a/strvec.c
+++ b/strvec.c
@@ -61,16 +61,18 @@ void strvec_splice(struct strvec *array, size_t idx, size_t len,
 {
 	if (idx + len > array->nr)
 		BUG("range outside of array boundary");
-	if (replacement_len > len)
+	if (replacement_len > len) {
+		if (array->v == empty_strvec)
+			array->v = NULL;
 		ALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,
 			   array->alloc);
+	}
 	for (size_t i = 0; i < len; i++)
 		free((char *)array->v[idx + i]);
-	if (replacement_len != len) {
+	if ((replacement_len != len) && array->nr)
 		memmove(array->v + idx + replacement_len, array->v + idx + len,
 			(array->nr - idx - len + 1) * sizeof(char *));
-		array->nr += (replacement_len - len);
-	}
+	array->nr += replacement_len - len;
 	for (size_t i = 0; i < replacement_len; i++)
 		array->v[idx + i] = xstrdup(replacement[i]);
 }
diff --git a/t/unit-tests/strvec.c b/t/unit-tests/strvec.c
index 855b602337..e66b7bbfae 100644
--- a/t/unit-tests/strvec.c
+++ b/t/unit-tests/strvec.c
@@ -88,6 +88,16 @@ void test_strvec__pushv(void)
 	strvec_clear(&vec);
 }
 
+void test_strvec__splice_just_initialized_strvec(void)
+{
+	struct strvec vec = STRVEC_INIT;
+	const char *replacement[] = { "foo" };
+
+	strvec_splice(&vec, 0, 0, replacement, ARRAY_SIZE(replacement));
+	check_strvec(&vec, "foo", NULL);
+	strvec_clear(&vec);
+}
+
 void test_strvec__splice_with_same_size_replacement(void)
 {
 	struct strvec vec = STRVEC_INIT;

Range-diff against v1:
1:  0b60fcc51a ! 1:  c1991e6f3c strvec: `strvec_splice()` to a statically initialized vector
    @@ Metadata
      ## Commit message ##
         strvec: `strvec_splice()` to a statically initialized vector
     
    -    Let's avoid an invalid pointer error in case a client of
    -    `strvec_splice()` ends up with something similar to:
    +    We use a singleton empty array to initialize a `struct strvec`;
    +    similar to the empty string singleton we use to initialize a `struct
    +    strbuf`.
    +
    +    Note that an empty strvec instance (with zero elements) does not
    +    necessarily need to be an instance initialized with the singleton.
    +    Let's refer to strvec instances initialized with the singleton as
    +    "empty-singleton" instances.
    +
    +        As a side note, this is the current `strvec_pop()`:
    +
    +        void strvec_pop(struct strvec *array)
    +        {
    +            if (!array->nr)
    +                    return;
    +            free((char *)array->v[array->nr - 1]);
    +            array->v[array->nr - 1] = NULL;
    +            array->nr--;
    +        }
    +
    +        So, with `strvec_pop()` an instance can become empty but it does
    +        not going to be the an "empty-singleton".
    +
    +    This "empty-singleton" circumstance requires us to be careful when
    +    adding elements to instances.  Specifically, when adding the first
    +    element:  when we detach the strvec instance from the singleton and
    +    set the internal pointer in the instance to NULL.  After this point we
    +    apply `realloc()` on the pointer.  We do this in
    +    `strvec_push_nodup()`, for example.
    +
    +    The recently introduced `strvec_splice()` API is expected to be
    +    normally used with non-empty strvec's.  However, it can also end up
    +    being used with "empty-singleton" strvec's:
     
                struct strvec arr = STRVEC_INIT;
    +           int a = 0, b = 0;
    +
    +           ... no modification to arr, a or b ...
    +
                const char *rep[] = { "foo" };
    +           strvec_splice(&arr, a, b, rep, ARRAY_SIZE(rep));
    +
    +    So, we'll try to add elements to an "empty-singleton" strvec instance.
     
    -           strvec_splice(&arr, 0, 0, rep, ARRAY_SIZE(rep));
    +    Avoid misapplying `realloc()` to the singleton in `strvec_splice()` by
    +    adding a special case for strvec's initialized with the singleton.
     
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
     
    @@ strvec.c: void strvec_splice(struct strvec *array, size_t idx, size_t len,
      			(array->nr - idx - len + 1) * sizeof(char *));
     -		array->nr += (replacement_len - len);
     -	}
    -+	array->nr += (replacement_len - len);
    ++	array->nr += replacement_len - len;
      	for (size_t i = 0; i < replacement_len; i++)
      		array->v[idx + i] = xstrdup(replacement[i]);
      }

-- 
2.47.0.281.g7eb946317c