Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 20, 2016, 22:40 UTC
Message-ID: <22e4b88b-5273-99c9-bcb3-0748c17a7229@google.com>
In-Reply-To: <xmqqvawmekhk.fsf@gitster.mtv.corp.google.com>

On 10/20/2016 03:14 PM, Junio C Hamano wrote:

Show 19 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>>> +static int find_separator(const char *line)
>>> +{
>>> +       const char *c;
>>> +       for (c = line; ; c++) {
>>> +               if (!*c || *c == '\n')
>>> +                       return -1;
>>> +               if (*c == '=' || strchr(separators, *c))
>>> +                       return c - line;
>>> +       }
>>
>> I was about to suggest this function can be simplified and maybe
>> even inlined by the use of strspn or strcspn, but I think manual
>> processing of the string is fine, too, as it would not really be shorter.
>
> Hmm, I fear that iterating over a line one-byte-at-a-time and
> running strchr(separators, *c) on it for each byte has a performance
> implication over running a single call to strcspn(line, separators).

If we do that, there is also the necessity of creating a string that combines the separators and '=' (I guess '\n' is not necessary now, since all the lines are null terminated). I'm OK either way.

(We could cache that string, although I would think that if we did that, we might as well write the loop manually, like in this patch.)