Re: [PATCH] create_delta_index: simplify condition always evaluating to true

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 15, 2013, 21:46 UTC
Message-ID: <CAPig+cQ5Y9irLk=9Bhz09c=5yzZEcyMKn2kbhcrO_zDpgmkhGw@mail.gmail.com>
In-Reply-To: <1376602462-32339-1-git-send-email-stefanbeller@googlemail.com>

On Thu, Aug 15, 2013 at 5:34 PM, Stefan Beller <stefanbeller@googlemail.com> wrote:

Show 15 quoted lines

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

Thanks. Adding the quotes around 'i' makes your meaning clear. Without the quotes, apparently it was ambiguous, and my brain read it as a misspelling of 'it'.

Show 8 quoted lines

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

Indeed, I feel uncomfortable with the patch in general for the very reason that you state: it might become live again. Without the patch, the code remains safe without any extra effort. With this patch, even with the in-code comment, someone making changes needs to take special care. Sometimes it makes sense to leave safeties in place, even if they can't be triggered _today_; and safeties (such as i < 31) also serve as documentation.

Show 24 quoted lines

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