Re: [PATCH v3 4/6] trailer: make args have their own struct

From: Stefan Beller <sbeller@google.com>
Date: Oct 17, 2016, 23:20 UTC
Message-ID: <CAGZ79kYuaHvO-aXeCocDseTLe13g=PesKxQgOrcOREtM1CsWcg@mail.gmail.com>
In-Reply-To: <2357e9e6418fc5b30a60efe16dd8f72bf7544fef.1476466609.git.jonathantanmy@google.com>

On Fri, Oct 14, 2016 at 10:38 AM, Jonathan Tan <jonathantanmy@google.com> wrote:

Show 29 quoted lines

> Improve type safety by making arguments (whether from configuration or
> from the command line) have their own "struct arg_item" type, separate
> from the "struct trailer_item" type used to represent the trailers in
> the buffer being manipulated.
>
> This change also prepares "struct trailer_item" to be further
> differentiated from "struct arg_item" in future patches.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  trailer.c | 135 +++++++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 85 insertions(+), 50 deletions(-)
>
> diff --git a/trailer.c b/trailer.c
> index 54cc930..a9ed3f8 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -29,6 +29,12 @@ struct trailer_item {
>         struct list_head list;
>         char *token;
>         char *value;
> +};
> +
> +struct arg_item {
> +       struct list_head list;
> +       char *token;
> +       char *value;
>         struct conf_info conf;
>  };

(Unrelated side note:) When first seeing this diff, I assumed the diff heuristic is going wild, because it doesn't add a full struct. But on a second closer look, I realize this is the only correct diff, because we do not account for moved lines from one struct to the other.

Show 9 quoted lines

>  static void add_arg_to_input_list(struct trailer_item *on_tok,
> -                                 struct trailer_item *arg_tok)
> +                                 struct arg_item *arg_tok)
>  {
> -       if (after_or_end(arg_tok->conf.where))
> -               list_add(&arg_tok->list, &on_tok->list);
> +       int aoe = after_or_end(arg_tok->conf.where);
> +       struct trailer_item *to_add = trailer_from_arg(arg_tok);
> +       if (aoe)

The use of an extra variable here is more readable than my imagined version of inlining to_add into the list_add calls just to save aoe.

Looks good to me.

Stefan