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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Rubén Justo <rjusto@xxxxxxxxx> writes:

Nit: Is the commit subject missing a verb?

> We use a singleton empty array to initialize a `struct strvec`,
> similar to the empty string singleton we use to initialize a `struct
> strbuf`.
>

So a singleton empty array is a statically allocated array element, so
for strvec, this would be `const char *empty_strvec[] = { NULL }`.

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

Right, so when we add elements ideally, we ideally check whether it is a
singleton or not. This is evident in `strvec_push_nodup()`:

    void strvec_push_nodup(struct strvec *array, char *value)
    {
    	if (array->v == empty_strvec)
    		array->v = NULL;

    	ALLOC_GROW(array->v, array->nr + 2, array->alloc);
    	array->v[array->nr++] = value;
    	array->v[array->nr] = NULL;
    }

>     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".

Correct, since we simply set the array element to NULL, but this is
still a dynamically allocated array.

Nit: The sentence reads a bit weirdly.

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

So everything said here makes sense, that's a great explanation.

> 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 :-)
>

I would say this is very useful over the first iteration, considering I
am someone without prior knowledge here.

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

Why is this second block of changes needed? Will array-nr ever be 0 when
we reach here?

>  	for (size_t i = 0; i < replacement_len; i++)
>  		array->v[idx + i] = xstrdup(replacement[i]);
>  }

[snip]

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux