Re: [PATCH 3/4] argv_array: add argv_array_copy

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Aug 6, 2015 at 1:35 PM, Stefan Beller <sbeller@google.com> wrote:
> The copied argv array shall be an identical deep copy except for
> the internal allocation value.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/argv-array.c b/argv-array.c
> index 256741d..6d9c1dd 100644
> --- a/argv-array.c
> +++ b/argv-array.c
> @@ -68,3 +68,16 @@ void argv_array_clear(struct argv_array *array)
> +void argv_array_copy(struct argv_array *src, struct argv_array *dst)

'src' should be 'const'.

Typical Unix argument order has 'dst' first and 'src' second, i.e
strcpy(dst, src). Is it worth deviating from that precedent?

> +{
> +       int i;
> +
> +       dst->argv = xmalloc((src->argc + 1) * sizeof(*dst->argv));

What happens if 'dst' already has content? Isn't that being leaked
here? At the very least, don't you want to argv_array_clear(dst)?

> +       dst->argc = src->argc;
> +       dst->alloc = src->argc;

This is wrong, of course. The number allocated is actually argc+1, not argc.

> +       for (i = 0; i < dst->argc ; i++)

While it's not wrong per se to use dst->argc as the terminating
condition, it is potentially misleading and confusing. Instead using
src->argc as the terminating condition will better telegraph that the
copy process is indeed predicated upon 'src'.

> +               dst->argv[i] = xstrdup(src->argv[i]);
> +       dst->argv[dst->argc] = NULL;

It's not clear why you want to hand-code the low-level functionality
again (such as array allocation and string duplication), risking (and
indeed making) errors in the process, when you could instead re-use
existing argv_array code. I would have expected to see
argv_array_copy() implemented as:

    argv_array_clear(dst);
    for (i = 0; i < src->argc; i++)
        argv_array_push(dst, src->argv[i]);

which provides far fewer opportunities for errors to creep in.

Moreover, this function might be too special-purpose. That is, why
does it need to overwrite 'dst'? Can't you achieve the same
functionality by merely appending to 'dst', and leave it up to the
caller to decide whether 'dst' should be cleared beforehand or not? If
so, then you can drop the argv_array_clear(dst) from the above.

However, that begs the question: Why do you need argv_array_copy() at
all? Isn't the same functionality already provided by
argv_array_pushv()? To wit, a caller which wants to copy from 'src' to
'dst' can already do:

    struct argv_array src = ...;
    struct argv_array dst = ARGV_ARRAY_INIT;
    argv_array_pushv(&dst, src->argv);

> +}
> +
> diff --git a/argv-array.h b/argv-array.h
> index c65e6e8..247627da 100644
> --- a/argv-array.h
> +++ b/argv-array.h
> @@ -19,5 +19,6 @@ LAST_ARG_MUST_BE_NULL
>  void argv_array_pushl(struct argv_array *, ...);
>  void argv_array_pop(struct argv_array *);
>  void argv_array_clear(struct argv_array *);
> +void argv_array_copy(struct argv_array *src, struct argv_array *dst);
>
>  #endif /* ARGV_ARRAY_H */
> --
> 2.5.0.239.g9728e1d.dirty