On Thu, Aug 15, 2013 at 5:34 PM, Stefan Beller <stefanbel...@googlemail.com> wrote: > 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 <stefanbel...@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'. > 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. > > 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 > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
