Re: [PATCH 1/3] apply: get rid of useless x < 0 comparison on a size_t type

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 6, 2011, 18:35 UTC
Message-ID: <7vsjm15cap.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1320581184-4557-2-git-send-email-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 19 quoted lines

> According to the C standard size_t is always unsigned, therefore the
> comparison "n1 < 0 || n2 < 0" when n1 and n2 are size_t will always be
> false.
>
> This was raised by clang 2.9 which throws this warning when compiling
> apply.c:
>
>     builtin/apply.c:253:9: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]
>             if (n1 < 0 || n2 < 0)
>                 ~~ ^ ~
>     builtin/apply.c:253:19: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]
>             if (n1 < 0 || n2 < 0)
>                           ~~ ^ ~
>
> This check was originally added in v1.6.5-rc0~53^2 by Giuseppe Bilotta
> while adding an option to git-apply to ignore whitespace differences.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---

I agree that these quantities can never be negative, so I'll apply the patch as is.

But I have this suspicion that this was a rather sloppy defensive check to protect this codepath against potential breakage in another codepath (most likely update_pre_post_images() touched by the same patch) that adjusts image->line[].len the caller of this function uses to feed these two parameters. Giuseppe may have been not confident enough that the code added to that function ensures not to undershoot when it reduces "len", or something.

Giuseppe, can you explain what is going on?

Thanks

Show 17 quoted lines

>  builtin/apply.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 84a8a0b..b3b59db 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;
> -
>  	/* ignore line endings */
>  	while ((*last1 == '\r') || (*last1 == '\n'))
>  		last1--;