{"thread":{"id":"36276","subject":"[PATCH v2] Rewrite strbuf.c:strbuf_cmp() replace memcmp() with starts_with()","startedAt":"2014-03-22T21:25:16Z","lastAt":"2014-03-23T09:02:06Z","messageCount":4,"participants":["Mustafa Orkun Acar","Alexandru Guduleasa","David Kastrup","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"237406","messageId":"1395523516-10181-1-git-send-email-mustafaorkunacar@gmail.com","threadId":"36276","inReplyTo":null,"subject":"[PATCH v2] Rewrite strbuf.c:strbuf_cmp() replace memcmp() with starts_with()","fromName":"Mustafa Orkun Acar","fromEmail":"mustafaorkunacar@gmail.com","sentAt":"2014-03-22T21:25:16Z","receivedAt":"2014-03-22T21:25:16Z","isPatch":true,"sender":{"key":"mustafaorkunacar@gmail.com","avatar":null},"body":"I reviewed all functions using memcmp(). It generally makes code more understandable. But here it might be used for the sake of simplicity.\n\nSigned-off-by: Mustafa Orkun Acar \n---\nI applied to GSoC 2014. I expect your feedbacks and comments!\n strbuf.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex ee96dcf..50d0875 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -147,7 +147,7 @@ void strbuf_list_free(struct strbuf **sbs)\n int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)\n {\n \tint len = a->len < b->len ? a->len: b->len;\n-\tint cmp = memcmp(a->buf, b->buf, len);\n+\tint cmp = !starts_with(a->buf, b->buf);\n \tif (cmp)\n \t\treturn cmp;\n \treturn a->len < b->len ? -1: a->len != b->len;\n-- \n1.9.1.286.g5172cb3\n"},{"id":"237407","messageId":"CA+ph3SoiNtX+ghTZsWetgAJ23SYYsAzyq+RSwHhtaxQhq-Pk7g@mail.gmail.com","threadId":"36276","inReplyTo":"1395523516-10181-1-git-send-email-mustafaorkunacar@gmail.com","subject":"Re: [PATCH v2] Rewrite strbuf.c:strbuf_cmp() replace memcmp() with starts_with()","fromName":"Alexandru Guduleasa","fromEmail":"alexandru.guduleasa@gmail.com","sentAt":"2014-03-22T21:58:02Z","receivedAt":"2014-03-22T21:58:02Z","isPatch":true,"sender":{"key":"alexandru.guduleasa@gmail.com","avatar":null},"body":"Hi,\n\nThis does not seam correct to me.\nThe memcmp function could have returned 3 relevant values (zero,\npositive and negative) and a non-zero result would have been returned\nby the if statement.\nWith you modification, you replace a negative value from memcmp with a\npositive one.\n\nBest regards,\nAlex Guduleasa\n"},{"id":"237408","messageId":"87fvm928ve.fsf@fencepost.gnu.org","threadId":"36276","inReplyTo":"1395523516-10181-1-git-send-email-mustafaorkunacar@gmail.com","subject":"Re: [PATCH v2] Rewrite strbuf.c:strbuf_cmp() replace memcmp() with starts_with()","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-03-22T21:58:45Z","receivedAt":"2014-03-22T21:58:45Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Mustafa Orkun Acar writes:\n\n> I reviewed all functions using memcmp(). It generally makes code more understandable. But here it might be used for the sake of simplicity.\n>\n> Signed-off-by: Mustafa Orkun Acar \n> ---\n> I applied to GSoC 2014. I expect your feedbacks and comments!\n> strbuf.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index ee96dcf..50d0875 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -147,7 +147,7 @@ void strbuf_list_free(struct strbuf **sbs)\n> int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)\n> {\n> \tint len = a->len < b->len ? a->len: b->len;\n> -\tint cmp = memcmp(a->buf, b->buf, len);\n> +\tint cmp = !starts_with(a->buf, b->buf);\n> \tif (cmp)\n> \t\treturn cmp;\n> \treturn a->len < b->len ? -1: a->len != b->len;\n\nNot correct. The original code clearly takes care to return a signed\nresult with the same definition of signedness as memcmp. While this\nintent has not been written down in a comment or description in either\nstrbuf.c or strbuf.h, the code does not make sense without it.\n\nrerere.c contains the following lines:\n\n if (strbuf_cmp(&one, &two) > 0)\n strbuf_swap(&one, &two);\n\nand that only makes sense when there is an actual meaning to the sign of\nthe result.\n\nYour version would return 1 when either comparing \"1\" with \"2\" OR \"2\"\nwith \"1\". It requires NUL-terminated strings: if that was a valid\nconstraint for strbuf, this function would be using strcmp in the first\nplace.\n\n-- \nDavid Kastrup\n"},{"id":"237414","messageId":"CAPig+cRVRSDktaQhXNHZfWJ+G=k6XhP6HxjJ4iOxsUfrDcYmWg@mail.gmail.com","threadId":"36276","inReplyTo":"1395523516-10181-1-git-send-email-mustafaorkunacar@gmail.com","subject":"Re: [PATCH v2] Rewrite strbuf.c:strbuf_cmp() replace memcmp() with starts_with()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-23T09:02:06Z","receivedAt":"2014-03-23T09:02:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"In addition to the valuable review comments already provided by\nAlexandru and David, see a few more below.\n\nOn Sat, Mar 22, 2014 at 5:25 PM, Mustafa Orkun Acar\n wrote:\n> Subject: [PATCH v2] Rewrite strbuf.c:strbuf_cmp() replace memcmp() with starts_with()\n\nThis isn't actually v2. It would have been v2 if it was a reroll of\nyour original patch [1], but this patch is entirely distinct from that\nattempt.\n\nTake a close look at the example Subject I wrote [2] in the review of\nyour first patch. Try to emulate it when writing the subject for your\npatches. (You seem to have ignored it for this patch.)\n\n> I reviewed all functions using memcmp(). It generally makes code more understandable. But here it might be used for the sake of simplicity.\n\nLikewise, re-read the review [2] of your original patch. In\nparticular, see the part about wrapping text to 65-70 characters\n(which you also seem to have ignored).\n\nThe sentence \"I reviewed all functions using memcmp()\" is primarily\ncommentary that won't be meaningful to someone reading the official\nproject history months or years from now. Place it below the \"---\"\nline under your sign-off.\n\nThe second and third sentences are somewhat weak. You might instead\nwant to say something about how starts_with() does a better job\nconveying the intention of the logic than does memcmp().\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/244529\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/244529/focus=244643\n\n> Signed-off-by: Mustafa Orkun Acar \n> ---\n> I applied to GSoC 2014. I expect your feedbacks and comments!\n> strbuf.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index ee96dcf..50d0875 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -147,7 +147,7 @@ void strbuf_list_free(struct strbuf **sbs)\n> int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)\n> {\n> int len = a->len < b->len ? a->len: b->len;\n> - int cmp = memcmp(a->buf, b->buf, len);\n> + int cmp = !starts_with(a->buf, b->buf);\n> if (cmp)\n> return cmp;\n> return a->len < b->len ? -1: a->len != b->len;\n> --\n> 1.9.1.286.g5172cb3\n>\n"}]}