threads / patch / 36276

v2Rewrite strbuf.c:strbuf_cmp() replace memcmp() with starts_with()

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

## tl;dr

4 messages between Mar 22, 2014 and Mar 23, 2014. Diffs are folded; open one to read it.

replies: 3people: 4as markdown or json

Mustafa Orkun Acar· Mar 22, 2014, 21:25 UTC · lore
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(-)
Show changes to strbuf.c +1 −1
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
Alexandru Guduleasa· Mar 22, 2014, 21:58 UTC · re: Mustafa Orkun Acar · lore

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

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

David Kastrup· Mar 22, 2014, 21:58 UTC · re: Mustafa Orkun Acar · lore

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

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
Eric Sunshine· Mar 23, 2014, 09:02 UTC · re: Mustafa Orkun Acar · lore

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

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

Show 22 quoted lines
> 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
>

← back to recent threads