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

From: David Kastrup <dak@gnu.org>
Date: Mar 12, 2014, 20:52 UTC
Message-ID: <87ob1bi1hp.fsf@fencepost.gnu.org>
In-Reply-To: <xmqqiorjky0a.fsf@gitster.dls.corp.google.com>

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

Show 23 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.
>
> 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.

Show 6 quoted lines

> 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