From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sun, 07 Feb 2016 09:12:16 GMT
Subject: Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom
Message-ID: <CAPig+cSvjd6hX3B1SEXW176Nr3Hz9wSLfkNubA6O6d47zbYSEg@mail.gmail.com>
In-Reply-To: <CAOLa=ZTFVLxiP5PXszwmS3xueitQJd3FhJ968hf58yvWX=qmGA@mail.gmail.com>

On Sun, Feb 7, 2016 at 4:01 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Sun, Feb 7, 2016 at 12:03 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sat, Feb 6, 2016 at 10:15 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> I think the code needs to be changed to:
>>>
>>> -               if ((!arg || len == arg - sp) &&
>>> +               if ((arg || len == ep - sp) &&
>>> +                   (!arg || len == arg - sp) &&
>>
>> For completeness, for people reading the mailing list archive, a
>> couple alternate fixes were presented elsewhere[1], with a personal
>> bias toward:
>>
>>     arg = memchr(...);
>>     if (!arg)
>>         arg = ep;
>
> There is a slight issue with this solution though, as you see 'arg'
> gets modified
> here, hence 'arg' passed to parser functions will never will null.
> [...]
> Else we could avoid this assignment and re-assignment by letting 'arg'
> hold the value it gets from memcmp(...) and use the solution provided
> by me or Ramsay (preferably)
>
> Ramsay's solution being
>
>                 arg = memchr(sp, ':', ep - sp);
> -               if ((!arg || len == arg - sp) &&
> +               if ((( arg && len == arg - sp)  ||
> +                    (!arg && len == ep - sp )) &&
>                     !memcmp(valid_atom[i].name, sp, len))
>                         break;

Yep, Ramsey's fix is preferable.