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

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

[Mustafa Orkun Acar]

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;
-- 
1.9.1.286.g5172cb3

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

[Alexandru Guduleasa]

Hi,

This does not seam correct to me.
The memcmp function could have returned 3 relevant values (zero,
positive and negative) and a non-zero result would have been returned
by the if statement.
With you modification, you replace a negative value from memcmp with a
positive one.

Best regards,
Alex Guduleasa

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

[David Kastrup]

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

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

[Eric Sunshine]

In addition to the valuable review comments already provided by
Alexandru and David, see a few more below.

On Sat, Mar 22, 2014 at 5:25 PM, Mustafa Orkun Acar
<mustafaorkunacar@gmail.com> wrote:
> Subject: [PATCH v2] Rewrite strbuf.c:strbuf_cmp() replace memcmp() with starts_with()

This isn't actually v2. It would have been v2 if it was a reroll of
your original patch [1], but this patch is entirely distinct from that
attempt.

Take a close look at the example Subject I wrote [2] in the review of
your first patch. Try to emulate it when writing the subject for your
patches. (You seem to have ignored it for this patch.)

> I reviewed all functions using memcmp(). It generally makes code more understandable. But here it might be used for the sake of simplicity.

Likewise, re-read the review [2] of your original patch. In
particular, see the part about wrapping text to 65-70 characters
(which you also seem to have ignored).

The sentence "I reviewed all functions using memcmp()" is primarily
commentary that won't be meaningful to someone reading the official
project history months or years from now. Place it below the "---"
line under your sign-off.

The second and third sentences are somewhat weak. You might instead
want to say something about how starts_with() does a better job
conveying the intention of the logic than does memcmp().

[1]: http://thread.gmane.org/gmane.comp.version-control.git/244529
[2]: http://thread.gmane.org/gmane.comp.version-control.git/244529/focus=244643

> 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;
> --
> 1.9.1.286.g5172cb3
>