From: Philip Oakley <philipoakley@iee.org>
Date: Thu, 15 Aug 2013 22:14:23 GMT
Subject: Re: [PATCH] create_delta_index: simplify condition always evaluating to true
Message-ID: <0F8B752CC3F241A9874E8A268A25D162@PhilipOakley>
In-Reply-To: <1376602462-32339-1-git-send-email-stefanbeller@googlemail.com>

From: "Stefan Beller" <stefanbeller@googlemail.com>
> When checking the previous lines in that function, we can deduce that
> hsize must always be smaller than (1u<<31), since 506049c7df2c6
> (fix >4GiB source delta assertion failure), because entries is
> capped at an upper bound of 0xfffffffeU, so hsize contains a maximum
> value of 0x3fffffff, which is smaller than (1u<<31), so the value of
> 'i' will never be larger than 31.
>
> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
> ---
>
> Eric, thanks for reviewing my patch.
>
> I applied the first 2 proposals (deduce, entries), but I disagree on
> the third, so I reformulated the sentence, as I really meant the 
> variable
> i and not it as a pronoun.
>
> Do I understand right, you're suggesting to remove the
> source code comment? I did this now, but I have a bad feeling with it.
>
> The change of this patch surely removes dead code as of now and makes 
> it
> more readable. But also it could become alive again, once somebody
> changes things nearby and forgets about the assumption, hsize not
> exceeding a certain size. That's why I put a comment in there, so
> the future changes nearby may be more careful.

Should the comment also include a note about potential undefined 
behaviour of a large shift, with the possible consequential bad compiler 
optimization, such as simply deleting the code. I understand the initial 
spot was part of the STACK tool check of such latent optimisation bugs.

http://css.csail.mit.edu/stack/

>
> Thanks,
> Stefan
>
>
> diff-delta.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/diff-delta.c b/diff-delta.c
> index 93385e1..3797ce6 100644
> --- a/diff-delta.c
> +++ b/diff-delta.c
> @@ -155,7 +155,7 @@ struct delta_index * create_delta_index(const void 
> *buf, unsigned long bufsize)
>  entries = 0xfffffffeU / RABIN_WINDOW;
>  }
>  hsize = entries / 4;
> - for (i = 4; (1u << i) < hsize && i < 31; i++);
> + for (i = 4; (1u << i) < hsize; i++);
>  hsize = 1 << i;
>  hmask = hsize - 1;
>
> -- 
> 1.8.4.rc3.1.gc1ebd90
>