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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]