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

[Eric Sunshine <sunshine@sunshineco.com>]

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).