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

[David Kastrup <dak@gnu.org>]

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

> 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