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: 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.
Avoid misapplying `realloc()` to the singleton in `strvec_splice()` by
adding a special case for "empty-singleton" strvec's.
Signed-off-by: Rubén Justo <rjusto@xxxxxxxxx>
---
This iteration adds more detail to the message plus a minor change to
remove some unnecessary parentheses.
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.
Rereading what I wrote last night, perhaps the problem now is excess.
I hope not. In any case, here it is :-)
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@xxxxxxxxx>
@@ 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
