Re: [PATCH v2] Rewrite strbuf.c:strbuf_cmp() replace memcmp() with starts_with()
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Mar 23, 2014, 09:02 UTC
- Message-ID
- <CAPig+cRVRSDktaQhXNHZfWJ+G=k6XhP6HxjJ4iOxsUfrDcYmWg@mail.gmail.com>
- In-Reply-To
- <1395523516-10181-1-git-send-email-mustafaorkunacar@gmail.com>
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
>