Re: RFC: C code cleanup

From: Junio C Hamano <gitster-vger@pobox.com>
Date: Jun 3, 2011, 23:49 UTC
Message-ID: <BANLkTikxY8QcGO70Lg0HQAVZ9kwmsD0WF6HEoGC1bojWo4Myog@mail.gmail.com>
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).

Show 11 quoted lines

> 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.

Show 9 quoted lines

> @@ -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

Show 17 quoted lines

> 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.