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

On Wed, Nov 25, 2015 at 7:10 AM, Karthik Nayak [off-list ref] wrote:

On Tue, Nov 24, 2015 at 5:14 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:

quoted

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?

I plan on filling that in upcoming patches. Probably, should mention that in
the commit message.

Making it clear that this patch is preparatory for introduction of
'valid_atom' is a good idea, however, adding the unused 'valid_atom'
field in this patch is not recommended. It would be better to
introduce 'valid_atom' in the patch which actually needs it.

quoted

quoted

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

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.

This is to make way for upcoming patches. But the compiler error is
accurate used_atom only becomes a structure in the next patch.
Should change that.

This problem will go away if you introduce the 'valid_atom' field in
the patch which actually needs it (as suggested above) rather than in
this patch.