Re: [PATCH/RFC 02/10] ref-filter: introduce struct used_atom
From: Eric Sunshine <hidden>
Date: 2016-06-15 23:07:21
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] 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/
quoted hunk ↗ jump to hunk
'used_array' for future usage. Signed-off-by: Karthik Nayak <redacted> ---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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
-/* * 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;