Re: [PATCH v2 03/11] ref-filter: introduce struct used_atom

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 16, 2015, 20:57 UTC
Message-ID: <CAPig+cR5=8urU+Vq_doC53+T4WCmEWJdKXyMEsbL3gWxsNpmxg@mail.gmail.com>
In-Reply-To: <1450279802-29414-4-git-send-email-Karthik.188@gmail.com>

On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak <karthik.188@gmail.com> wrote:

> Introduce the 'used_array' structure which would replace the existing
> implementation of 'used_array' (which a list of atoms). This helps us
> parse atom's before hand and store required details into the
> 'used_array' for future usage.

All my v1 review comments[1] about the commit message still apply to this version.

[1]: http://article.gmane.org/gmane.comp.version-control.git/281860

> Also introduce a parsing function for each atom in valid_atom. Using
> this we can define special parsing functions for each of the atoms.

This is a conceptually distinct change which probably deserves its own patch. In particular, the new patch would add this field to valid_atom[] *and* add the code which invokes the custom parser. (That code is currently commingled with introduction of the color parser in patch 6/11.)

More below...

Show 35 quoted lines

> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -16,9 +16,27 @@
> +/*
> + * An atom is a valid field atom listed below, possibly prefixed with
> + * a "*" to denote deref_tag().
> + *
> + * We parse given format string and sort specifiers, and make a list
> + * of properties that we need to extract out of objects.  ref_array_item
> + * structure will hold an array of values extracted that can be
> + * indexed with the "atom number", which is an index into this
> + * array.
> + */
> +static struct used_atom {
> +       const char *str;
> +       cmp_type type;
> +} *used_atom;
> +static int used_atom_cnt, need_tagged, need_symref;
> +static int need_color_reset_at_eol;
> +
>  static struct {
>         const char *name;
>         cmp_type cmp_type;
> +       void (*parser)(struct used_atom *atom);
>  } valid_atom[] = {
>         { "refname" },
>         { "objecttype" },
> @@ -786,7 +788,8 @@ static void populate_value(struct ref_array_item *ref)
>
>         /* Fill in specials first */
>         for (i = 0; i < used_atom_cnt; i++) {
> -               const char *name = used_atom[i];
> +               struct used_atom *atom = &used_atom[i];
> +               const char *name = atom->str;

Same question as my previous review[1]: Why not just:

    const char *name = used_atom[i].str;

>                 struct atom_value *v = &ref->value[i];
>                 int deref = 0;
>                 const char *refname;