Re: [PATCH] general style: replaces memcmp() with proper starts_with()

From: René Scharfe <l.s.r@web.de>
Date: Mar 12, 2014, 20:51 UTC
Message-ID: <5320C8CC.3010404@web.de>
In-Reply-To: <xmqqiorjky0a.fsf@gitster.dls.corp.google.com>

Am 12.03.2014 20:39, schrieb Junio C Hamano:

Show 18 quoted lines

> Jeff King <peff@peff.net> writes:
>
>>>   static inline int standard_header_field(const char *field, size_t len)
>>>   {
>>> -	return ((len == 4 && !memcmp(field, "tree ", 5)) ||
>>> -		(len == 6 && !memcmp(field, "parent ", 7)) ||
>>> -		(len == 6 && !memcmp(field, "author ", 7)) ||
>>> -		(len == 9 && !memcmp(field, "committer ", 10)) ||
>>> -		(len == 8 && !memcmp(field, "encoding ", 9)));
>>> +	return ((len == 4 && starts_with(field, "tree ")) ||
>>> +		(len == 6 && starts_with(field, "parent ")) ||
>>> +		(len == 6 && starts_with(field, "author ")) ||
>>> +		(len == 9 && starts_with(field, "committer ")) ||
>>> +		(len == 8 && starts_with(field, "encoding ")));
>>
>> These extra "len" checks are interesting.  They look like an attempt to
>> optimize lookup, since the caller will already have scanned forward to
>> the space.

I wonder what the performance impact might be. The length checks are also needed for correctness, however, to avoid running over the end of the buffer.

Show 8 quoted lines

> If one really wants to remove the magic constants from this, then
> one must take advantage of the pattern
>
> 	len == strlen(S) - 1 && !memcmp(field, S, strlen(S))
>
> that appears here, and come up with a simple abstraction to express
> that we are only using the string S (e.g. "tree "), length len and
> location field of the counted string.

This might be a job for kwset.

René