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 25, 2015, 19:41 UTC
- Message-ID
- <CAPig+cRcxr7uT2OJX=TnaVf+gXQdw9ydp+7X+Kis4Vb5+6RHiA@mail.gmail.com>
- In-Reply-To
- <CAOLa=ZTFeKMMSRCTWi9RkSvuCh7ZecPudSgsJB4TV76U4aGJJA@mail.gmail.com>
On Wed, Nov 25, 2015 at 7:10 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
Show 13 quoted lines
> On Tue, Nov 24, 2015 at 5:14 AM, Eric Sunshine <sunshine@sunshineco.com> wrote: >> 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? > > 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.
Show 21 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.
>
> 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.