Re: [PATCH v3 10/15] ref-filter: introduce parse_align_position()

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 25, 2016, 21:49 UTC
Message-ID: <CAPig+cSUBQiW396qyiczcdPV9qU9o_8imuL89O6jLWLY8sX2-A@mail.gmail.com>
In-Reply-To: <1451980994-26865-11-git-send-email-Karthik.188@gmail.com>

On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak <karthik.188@gmail.com> wrote:

Show 20 quoted lines

> From align_atom_parser() form parse_align_position() which given a
> string would give us the alignment position. This is a preparatory
> patch as to introduce prefixes for the %(align) atom and avoid
> redundancy in the code.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -74,6 +74,17 @@ static void color_atom_parser(struct used_atom *atom)
> +static align_type parse_align_position(const char *s)
> +{
> +       if (!strcmp(s, "right"))
> +               return ALIGN_RIGHT;
> +       else if (!strcmp(s, "middle"))
> +               return ALIGN_MIDDLE;
> +       else if (!strcmp(s, "left"))
> +               return ALIGN_LEFT;
> +       return -1;
> +}

This code was just moved in patch 9/15 and is being relocated again here in patch 10/15. If you change the order of the patches so that this preparatory refactoring is done first, the diff of the "introduce align_atom_parser()" patch will become smaller and be a bit easier to review. (Plus it just makes sense to do preparation first.)

Show 25 quoted lines

>  static void align_atom_parser(struct used_atom *atom)
>  {
>         struct align *align = &atom->u.align;
> @@ -90,16 +101,13 @@ static void align_atom_parser(struct used_atom *atom)
>         align->position = ALIGN_LEFT;
>
>         while (*s) {
> +               int position;
>                 buf = s[0]->buf;
>
>                 if (!strtoul_ui(buf, 10, (unsigned int *)&width))
>                         ;
> -               else if (!strcmp(buf, "left"))
> -                       align->position = ALIGN_LEFT;
> -               else if (!strcmp(buf, "right"))
> -                       align->position = ALIGN_RIGHT;
> -               else if (!strcmp(buf, "middle"))
> -                       align->position = ALIGN_MIDDLE;
> +               else if ((position = parse_align_position(buf)) >= 0)
> +                       align->position = position;
>                 else
>                         die(_("unrecognized %%(align) argument: %s"), buf);
>                 s++;
> --
> 2.6.4