Re: [PATCH v16 05/14] ref-filter: introduce match_atom_name()

> On Sat, Sep 5, 2015 at 2:52 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Introduce match_atom_name() which helps in checking if a particular
>> atom is the atom we're looking for and if it has a value attached to
>> it or not.
>>
>> Use it instead of starts_with() for checking the value of %(color:...)
>> atom. Write a test for the same.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index a993216..e99c342 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> +static int match_atom_name(const char *name, const char *atom_name, const char **val)
>> +{
>> +       const char *body;
>> +
>> +       if (!skip_prefix(name, atom_name, &body))
>> +               return 0; /* doesn't even begin with "atom_name" */
>> +       if (!body[0] || !body[1]) {
>> +               *val = NULL; /* %(atom_name) and no customization */
>> +               return 1;
>
> If this is invoked as match_atom_name("colors", "color", ...), then it
> will return true (matched, but no value), which is not correct at all;
> "colors" is not a match for atom %(color). Maybe you meant?
>
>     if (!body[0] || (body[0] == ':' && !body[1])) {
>
> But, that's getting ugly and complicated, and would be bettered
> handled by reordering the logic of this function for dealing with the
> various valid and invalid cases. However...
>

>> +       }
>> +       if (body[0] != ':')
>> +               return 0; /* "atom_namefoo" is not "atom_name" or "atom_name:..." */
>> +       *val = body + 1; /* "atomname:val" */
>> +       return 1;
>> +}
>
> It's not clear why this function exists in the first place. It's only
> purpose seems to be to specially recognize instances of atoms which
> should have a ":" but lack it, so that the caller can then report an
> error.
>
> But why is this better than the more generic solution of merely
> reporting an error for *any* unrecognized atom? How does it help to
> single out these special cases?
>
> There is a further inconsistency in that you are only specially
> recognizing %(color) and %(align) lacking ":", but not
> %(content:lines) lacking "=" in patch 8/14. Why do %(color:...) and
> %(align:...) get special treatment but %(content:lines=...) does not?
> Such inconsistency again argues in favor of a generic "unrecognized
> atom" detection and reporting over special case handling.
>