Re: [PATCH/RFC 02/10] ref-filter: introduce struct used_atom
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Dec 1, 2015, 23:00 UTC
- Message-ID
- <CAPig+cRthCCK51XO2mju2VBe6Yvhy_Rp9nVvVUiWd9LuT9BxLQ@mail.gmail.com>
- In-Reply-To
- <1447271075-15364-3-git-send-email-Karthik.188@gmail.com>
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Introduce the 'used_array' structure which would replace the existing
I guess you meant s/used_array/used_atom/ or something?
Also, s/which would/to/
> implementation of 'used_array' (which a list of atoms). This helps us
s/which a/which is a/
> parse atom's before hand and store required details into the
s/atom's/atoms/ s/before hand/beforehand/
Show 18 quoted lines
> 'used_array' for future usage.
>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -16,6 +16,23 @@
> +/*
> + * 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;This is really the atom's name, isn't it? If so, perhaps "name" would be a better field name.
Show 25 quoted lines
> + 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;
> @@ -93,21 +110,6 @@ struct atom_value {
> };
>
> /*
> - * An atom is a valid field atom listed above, 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 const char **used_atom;
> -static cmp_type *used_atom_type;
> -static int used_atom_cnt, need_tagged, need_symref;
> -static int need_color_reset_at_eol;You're moving this block of declarations up above the valid_atom[] array because the previous patch added a new field named "parser" to valid_atom[] which references 'struct used_atom' added by this patch (2). I wonder if this movement should be done as a separate preparatory patch to make it easier to review since, as it stands, the reviewer has to read much more carefully to detect changes in the moved block.
Show 11 quoted lines
> -/*
> * Used to parse format string and sort specifiers
> */
> int parse_ref_filter_atom(const char *atom, const char *ep)
> @@ -787,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;Why not just:
const char *name = used_atom[i].str;
?
> struct atom_value *v = &ref->value[i]; > int deref = 0; > const char *refname;