On Thu, Nov 26, 2015 at 1:11 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Wed, Nov 25, 2015 at 7:10 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>> On Tue, Nov 24, 2015 at 5:14 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>>> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak <karthik.188@xxxxxxxxx> 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.
>
I get your point, will do as you suggested.
>>>> Signed-off-by: Karthik Nayak <Karthik.188@xxxxxxxxx>
>>>> ---
>>>> 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.
Yup, agreed.
Thanks for your suggestions.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html