Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

> On Thu, Feb 4, 2016 at 3:49 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> -               const char *formatp = strchr(sp, ':');
>>> -               if (!formatp || ep < formatp)
>>> -                       formatp = ep;
>>> -               if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
>>> +               arg = memchr(sp, ':', ep - sp);
>>
>> Why this change from strchr() to memchr()? I understand that you're
>> taking advantage of the fact that you know the extent of the string
>> via 'sp' and 'ep', however, was the original strchr() doing extra
>> work? Even if this change is desirable, it seems somewhat unrelated to
>> the overall purpose of this patch, thus might deserves its own.
>>
>> Aside from that, although the "expensive" strchr() / memchr() resides
>> within the loop, it will always return the same value since it doesn't
>> depend upon any condition local to the loop. This implies that it
>> ought to be hoisted out of the loop. (This problem is not new to this
>> patch; it's already present in the existing code.)
>
> I'm thinking I'll make a patch for that separately. i.e remove strchr()
> and introduce memchr() outside the loop.

>>> +               if ((!arg || len == arg - sp) &&
>>> +                   !memcmp(valid_atom[i].name, sp, len))
>>>                         break;
>>>         }
>>>
>>> @@ -154,6 +155,10 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>>>         REALLOC_ARRAY(used_atom, used_atom_cnt);
>>>         used_atom[at].name = xmemdupz(atom, ep - atom);
>>>         used_atom[at].type = valid_atom[i].cmp_type;
>>> +       if (arg)
>>> +               arg = used_atom[at].name + (arg - atom) + 1;
>>
>> This is a harder to understand than it ought to be because it's
>> difficult to tell at first glance that you don't actually care about
>> 'arg' in relation to the original incoming string, but instead use it
>> only to compute an offset into the string which is ultimately stored
>> in the newly allocated used_atom[]. Re-using 'arg' for a different
>> purpose (in a manner of speaking) confuses the issue further.
>>
>> The intention might be easier to follow if you made it clear that you
>> were interested in the *offset* of the argument in the string, rather
>> than a pointer into the incoming string which you ultimately don't
>> use. A variable named 'arg_offset' might go a long way toward
>> clarifying this intention.
>
> I hope you mean something like this.
>
> if (arg) {
>     int arg_offset;
>
>     arg_offset = (arg - atom) + 1;
>     arg = used_atom[at].name + arg_offset;
> }