Re: [PATCH/RFC 01/10] ref-filter: introduce a parsing function for each atom in valid_atom

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 23, 2015, 23:44 UTC
Message-ID: <CAPig+cRYEmbsye78ESOLfmSi56sBFeKpaEkpGSEwF=qrLZWx8g@mail.gmail.com>
In-Reply-To: <1447271075-15364-2-git-send-email-Karthik.188@gmail.com>

On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak <karthik.188@gmail.com> wrote:

> Introduce a parsing function for each atom in valid_atom. Using this
> we can define special parsing functions for each of the atoms. Since
> we have a third field in valid_atom structure, we now fill out missing
> cmp_type values.

I don't get it. Why do you need to "fill out missing cmp_type values" considering that you're never assigning the third field in this patch? Are you planning on filling in the third field in a future patch?

Show 8 quoted lines

> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -19,42 +19,43 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>  static struct {
>         const char *name;
>         cmp_type cmp_type;
> +       void (*parser)(struct used_atom *atom);

Compiler diagnostic:

    warning: declaration of 'struct used_atom' will not be
        visible outside of this function [-Wvisibility]

Indeed, it seems rather odd to introduce the new field in this patch but never actually do anything with it. It's difficult to understand the intention.

Show 70 quoted lines

>  } valid_atom[] = {
> -       { "refname" },
> -       { "objecttype" },
> +       { "refname", FIELD_STR },
> +       { "objecttype", FIELD_STR },
>         { "objectsize", FIELD_ULONG },
> -       { "objectname" },
> -       { "tree" },
> -       { "parent" },
> +       { "objectname", FIELD_STR },
> +       { "tree", FIELD_STR },
> +       { "parent", FIELD_STR },
>         { "numparent", FIELD_ULONG },
> -       { "object" },
> -       { "type" },
> -       { "tag" },
> -       { "author" },
> -       { "authorname" },
> -       { "authoremail" },
> +       { "object", FIELD_STR },
> +       { "type", FIELD_STR },
> +       { "tag", FIELD_STR },
> +       { "author", FIELD_STR },
> +       { "authorname", FIELD_STR },
> +       { "authoremail", FIELD_STR },
>         { "authordate", FIELD_TIME },
> -       { "committer" },
> -       { "committername" },
> -       { "committeremail" },
> +       { "committer", FIELD_STR },
> +       { "committername", FIELD_STR },
> +       { "committeremail", FIELD_STR },
>         { "committerdate", FIELD_TIME },
> -       { "tagger" },
> -       { "taggername" },
> -       { "taggeremail" },
> +       { "tagger", FIELD_STR },
> +       { "taggername", FIELD_STR },
> +       { "taggeremail", FIELD_STR },
>         { "taggerdate", FIELD_TIME },
> -       { "creator" },
> +       { "creator", FIELD_STR },
>         { "creatordate", FIELD_TIME },
> -       { "subject" },
> -       { "body" },
> -       { "contents" },
> -       { "upstream" },
> -       { "push" },
> -       { "symref" },
> -       { "flag" },
> -       { "HEAD" },
> -       { "color" },
> -       { "align" },
> -       { "end" },
> +       { "subject", FIELD_STR },
> +       { "body", FIELD_STR },
> +       { "contents", FIELD_STR },
> +       { "upstream", FIELD_STR },
> +       { "push", FIELD_STR },
> +       { "symref", FIELD_STR },
> +       { "flag", FIELD_STR },
> +       { "HEAD", FIELD_STR },
> +       { "color", FIELD_STR },
> +       { "align", FIELD_STR },
> +       { "end", FIELD_STR },
>  };
>
>  #define REF_FORMATTING_STATE_INIT  { 0, NULL }
> --
> 2.6.2