Re: [PATCH v2 11/11] ref-filter: introduce objectname_atom_parser()

[Karthik Nayak <karthik.188@gmail.com>]

On Fri, Dec 25, 2015 at 11:39 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Dec 25, 2015 at 8:44 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Fri, Dec 18, 2015 at 11:54 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>> +static void objectname_atom_parser(struct used_atom *atom)
>>>> +{
>>>> +       const char * buf;
>>>> +
>>>> +       if (match_atom_name(atom->str, "objectname", &buf))
>>>> +               atom->u.objectname = O_FULL;
>>>> +       if (!buf)
>>>> +               return;
>>>
>>> make_atom_name("objectname") will return true only for "objectname" or
>>> "objectname:", and will return false for anything else, such as
>>> "objectnamely" or "schmorf". Furthermore, the only way
>>> objectname_atom_parser() can be called is when %(objectname) or
>>> %(objectname:...) is seen, thus match_atom_name() *must* return true
>>> here, which means the above conditional is misleading, suggesting that
>>> it could somehow return false.
>>>
>>> And, if match_atom_name() did return false here, then that indicates a
>>> programming error: objectname_atom_parser() somehow got called for
>>> something other than %(objectname) or %(objectname:...). This implies
>>> that the code should instead be structured like this:
>>>
>>>     if (!match_atom_name(..., "objectname", &buf)
>>>         die("BUG: parsing non-'objectname'")
>>>     if (!buf)
>>>         atom->u.objectname = O_FULL;
>>>     else if (!strcmp(buf, "short"))
>>>         atom->u.objectname = O_SHORT;
>>>     else
>>>         die(_("unrecognized %%(objectname) argument: %s"), buf);
>>>
>>> However, this can be simplified further by recognizing that, following
>>> this patch series, match_atom_name() is *only* called by these new
>>> parse functions[1], which means that, as a convenience,
>>> match_atom_name() itself could become a void rather than boolean
>>> function and die() if the expected atom name is not found. Thus, the
>>> code would become:
>>>
>>>     match_atom_name(...);
>>>     if (!buf)
>>>         ...
>>>     else if (!strcmp(...))
>>>         ...
>>>     ...
>>>
>>> By the way, the above commentary isn't specific to this patch and
>>> %(objectname), but is in fact also relevant for all of the preceding
>>> patches which introduce parse functions calling match_atom_name().
>>
>> Ah! Thats some good observation, makes sense, match_atom_name()
>> is only called after the atom name, so making it return a indication of
>> success or failure doesn't make sense.
>>
>> I think this change would go nicely with the introduction of 'enum atom_type'
>> which you mentioned[0] in the previous series.
>
> I'm not sure to which change you refer as going nicely with the 'enum
> atom_type'.
>
> The observation above is about misleading logic in this series which
> makes the code confusing. At the very least, the present series should
> clean up the logic to reflect the first example above.
>
> Changing match_atom_name() from boolean to void, as in the second
> example above, is a nice little cleanup, but less important and
> needn't be in this series (though it would just be one minor patch at
> the end of the series).

I was referring to the second part as you just mentioned, But I could
at it to the
end of the series, I have restructured things as you mentioned in the example
though. Sorry for the confusion :)

-- 
Regards,
Karthik Nayak