Re: [PATCH] rewrite skip_prefix() as loop

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 27, 2014, 19:34 UTC
Message-ID: <xmqqha7kz6wh.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1393502562-28025-1-git-send-email-faizkothari@gmail.com>

Faiz Kothari <faiz.off93@gmail.com> writes:

> From: Faiz Kothari <faiz.off93@gmail.com>

Notice that this matches From: in your e-mail message, which means it is unnecessary. Drop it.

>
>
> Signed-off-by: Faiz Kothari <django@dj-pc.(none)>

And make sure this matches how you call yourself above.

Show 17 quoted lines

> ---
>  git-compat-util.h |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index cbd86c3..bb2582a 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -357,8 +357,11 @@ extern int suffixcmp(const char *str, const char *suffix);
>  
>  static inline const char *skip_prefix(const char *str, const char *prefix)
>  {
> -	size_t len = strlen(prefix);
> -	return strncmp(str, prefix, len) ? NULL : str + len;
> +	for (; ; str++, prefix++)
> +		if (!*prefix)
> +			return str;//code same as strbuf.c:starts_with()

Drop that comment. As already has been pointed out, say that in the proposed commit log message, perhaps like this:

	Subject: skip_prefix(): rewrite as a loop

	Instead of letting strlen() to scan the prefix once and then
        strncmp() to scan it again, rewrite it as a single loop,
        using the logic similar to starts_with() in strbuf.c.

	Signed-off-by: ...

> +		else if (*str != *prefix)
> +			return NULL;
>  }

This for() loop that does not have a loop-control condition looks a bit weird, though. If we were to use for() that is not "for (;;)", it would be more natural to write something like this:

	for (; *prefix && *str == *prefix; prefix++, str++)
		; /* keep going while they match */
	... decide what to return here ...

but that would make you check *prefix/*str twice for the final round, so it is not very optimal. If we want to say "the loop is endless and we exit explicitly from inside with our own logic", then

	while (1) {
		... what you have inside the loop ...
		prefix++;
                str++;
        }

would be easier on the eyes (you can do s/while (1)/for (;;)/, too).

Thanks.