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

On Thu, Dec 17, 2015 at 2:27 AM, Eric Sunshine [off-list ref] wrote:

On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak [off-list ref] wrote:

quoted

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

I totally missed this out, thanks for bringing it up.

quoted

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.)
go

I guess that could be done, I was thinking it goes together, but it
makes sense to have a
separate patch for introduction of the parsing function and its invocations.

More below...

quoted

Signed-off-by: Karthik Nayak <redacted>
---

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;

?

I think It's leftover code, I was using the atom variable also before.
I'll remove it.

-- 
Regards,
Karthik Nayak