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

[David Kastrup <dak@gnu.org>]

Junio C Hamano <gitster@pobox.com> writes:

> 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.
>
> 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))

If the caller has already scanned forward to the space, then there is no
actual need to let the comparison compare the space again after checking
len, is there?  That makes for a more consistent look in the memcmp
case.

One could do this in a switch on len instead.  Not that it seems
terribly more efficient.

> 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.
>
> Blindly replacing starts_with() with !memcmp() in the above part is
> a readability regression otherwise.

Don't really think so: if the len at the front and the back is the same
and the space is not explicitly compared any more, both look pretty much
the same to me.

>> ... I think with a few more helpers we could really further clean up
>> some of these callsites.
>
> Yes.

Possibly.  But it does seem like overkill.

-- 
David Kastrup