From: Jonathan Tan Date: Thu, 20 Oct 2016 22:45:34 GMT Subject: Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer Message-ID: In-Reply-To: <22e4b88b-5273-99c9-bcb3-0748c17a7229@google.com> On 10/20/2016 03:40 PM, Jonathan Tan wrote: > On 10/20/2016 03:14 PM, Junio C Hamano wrote: >> Stefan Beller 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.) Actually I guess we could generate the separators_and_equal string whenever we obtain new separators from the config. I'll do this in the next reroll.