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

[Stefan Beller <sbeller@google.com>]

On Fri, Oct 14, 2016 at 10:38 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
> 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.


>  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