Re: [PATCH v2] Rewrite strbuf.c:strbuf_cmp() replace memcmp() with starts_with()

From: David Kastrup <dak@gnu.org>
Date: Mar 22, 2014, 21:58 UTC
Message-ID: <87fvm928ve.fsf@fencepost.gnu.org>
In-Reply-To: <1395523516-10181-1-git-send-email-mustafaorkunacar@gmail.com>

Mustafa Orkun Acar <mustafaorkunacar@gmail.com> writes:

Show 21 quoted lines

> I reviewed all functions using memcmp(). It generally makes code more understandable. But here it might be used for the sake of simplicity.
>
> Signed-off-by: Mustafa Orkun Acar <mustafaorkunacar@gmail.com>
> ---
> I applied to GSoC 2014. I expect your feedbacks and comments!
>  strbuf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/strbuf.c b/strbuf.c
> index ee96dcf..50d0875 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -147,7 +147,7 @@ void strbuf_list_free(struct strbuf **sbs)
>  int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
>  {
>  	int len = a->len < b->len ? a->len: b->len;
> -	int cmp = memcmp(a->buf, b->buf, len);
> +	int cmp = !starts_with(a->buf, b->buf);
>  	if (cmp)
>  		return cmp;
>  	return a->len < b->len ? -1: a->len != b->len;

Not correct. The original code clearly takes care to return a signed result with the same definition of signedness as memcmp. While this intent has not been written down in a comment or description in either strbuf.c or strbuf.h, the code does not make sense without it.

rerere.c contains the following lines:

                        if (strbuf_cmp(&one, &two) > 0)
                                strbuf_swap(&one, &two);

and that only makes sense when there is an actual meaning to the sign of the result.

Your version would return 1 when either comparing "1" with "2" OR "2" with "1". It requires NUL-terminated strings: if that was a valid constraint for strbuf, this function would be using strcmp in the first place.

-- 
David Kastrup