git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster-vger@pobox.com>
To: Andy Lester <andy@petdance.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: RFC: C code cleanup
Date: Fri, 3 Jun 2011 16:49:15 -0700	[thread overview]
Message-ID: <BANLkTikxY8QcGO70Lg0HQAVZ9kwmsD0WF6HEoGC1bojWo4Myog@mail.gmail.com> (raw)
In-Reply-To: <3ECEA53B-C82C-4F3D-9E40-1D81EC17682E@petdance.com>

Thanks. The ones I don't comment on below looked like sensible changes
(except that their log messages need to conform to the project's
style).

> diff --git a/test-date.c b/test-date.c
> index 6bcd5b0..42642ed 100644
> --- a/test-date.c
> +++ b/test-date.c
> @@ -16,7 +16,7 @@ static void show_dates(char **argv, struct timeval *now)
>        }
>  }
>
> -static void parse_dates(char **argv, struct timeval *now)
> +static void parse_dates(char **argv)
>  {

As parse-dates mode does not handle any "relative" dates now, the
parameter happens not to be used in today's code. But judging from the
caller and other callers in the same if/else cascade, it probably is
better to leave this as-is.

> @@ -61,7 +61,7 @@ int main(int argc, char **argv)
>        if (!strcmp(*argv, "show"))
>                show_dates(argv+1, &now);
>        else if (!strcmp(*argv, "parse"))
> -               parse_dates(argv+1, &now);
> +               parse_dates(argv+1);
>        else if (!strcmp(*argv, "approxidate"))
>                parse_approxidate(argv+1, &now);
>        else

> commit 151ad9c45f08aa81598664e6e198af881fe52b77
> Author: Andy Lester <andy@petdance.com>
> Date:   Wed Jun 1 23:16:10 2011 -0500
>
>    Removed unnecessary test in fuzzy_matchlines(). size_t can never be negative
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 530d4bb..7e6fa4d 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -250,9 +250,6 @@ static int fuzzy_matchlines(const char *s1, size_t n1,
>        const char *last2 = s2 + n2 - 1;
>        int result = 0;
>
> -       if (n1 < 0 || n2 < 0)
> -               return 0;
> -

Looks like the author wanted to assert() to make sure the length
adjustments were not screwed up (a possible bug can be to update *.len
by subtracting too many, causing it to wrap around to become a
negative value but because size_t is unsigned, this is a wrong test to
catch it).

Perhaps casting these to ssize_t and dying with die("BUG: len
adjustment error") would be a better fix that is more faithful to the
intention of the original.

      parent reply	other threads:[~2011-06-03 23:49 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-06-03  3:32 RFC: C code cleanup Andy Lester
2011-06-03 16:02 ` Drew Northup
2011-06-03 21:38   ` Andy Lester
2011-06-03 23:49 ` Junio C Hamano [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BANLkTikxY8QcGO70Lg0HQAVZ9kwmsD0WF6HEoGC1bojWo4Myog@mail.gmail.com \
    --to=gitster-vger@pobox.com \
    --cc=andy@petdance.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).