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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 6, 2015, 18:18 UTC
Message-ID: <CAPig+cQ3bAML7WMyS3Q1YQCjR2SfgB47AEkTmqbycAJcig0=tg@mail.gmail.com>
In-Reply-To: <1438882524-21215-4-git-send-email-sbeller@google.com>

On Thu, Aug 6, 2015 at 1:35 PM, Stefan Beller <sbeller@google.com> wrote:

Show 11 quoted lines

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

Show 15 quoted lines

> +}
> +
> 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