{"thread":{"id":"34704","subject":"[PATCH] create_delta_index: simplify condition always evaluating to true","startedAt":"2013-08-15T19:37:40Z","lastAt":"2013-08-17T06:58:39Z","messageCount":14,"participants":["Stefan Beller","Eric Sunshine","Junio C Hamano","Philip Oakley","Nicolas Pitre","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"225279","messageId":"1376595460-6546-1-git-send-email-stefanbeller@googlemail.com","threadId":"34704","inReplyTo":null,"subject":"[PATCH] create_delta_index: simplify condition always evaluating to true","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-08-15T19:37:40Z","receivedAt":"2013-08-15T19:37:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When checking the previous lines in that function, we can deduct that\nhsize must always be smaller than (1u<<31), since 506049c7df2c6\n(fix >4GiB source delta assertion failure), because the entries is\ncapped at an upper bound of 0xfffffffeU, so hsize contains a maximum\nvalue of 0x3fffffff, which is smaller than (1u<<31), so i will never\nbe larger than 31.\n\nSigned-off-by: Stefan Beller \n---\n diff-delta.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/diff-delta.c b/diff-delta.c\nindex 93385e1..54da95b 100644\n--- a/diff-delta.c\n+++ b/diff-delta.c\n@@ -154,8 +154,15 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)\n \t\t */\n \t\tentries = 0xfffffffeU / RABIN_WINDOW;\n \t}\n+\n+\t/*\n+\t * Do not check i < 31 in the loop, because the assignement\n+\t * previous to the loop makes sure, hsize is definitely\n+\t * smaller than 1<<31, hence the loop will always stop\n+\t * before i exceeds 31 resulting in an infinite loop.\n+\t */\n \thsize = entries / 4;\n-\tfor (i = 4; (1u << i) < hsize && i < 31; i++);\n+\tfor (i = 4; (1u << i) < hsize; i++);\n \thsize = 1 << i;\n \thmask = hsize - 1;\n \n-- \n1.8.4.rc3.498.g5af1768\n"},{"id":"225283","messageId":"CAPig+cQmdPo4mo69DsDmUURcw+HbxkAoNEqY08qiuJs8S+=bvQ@mail.gmail.com","threadId":"34704","inReplyTo":"1376595460-6546-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH] create_delta_index: simplify condition always evaluating to true","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-08-15T21:21:03Z","receivedAt":"2013-08-15T21:21:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 15, 2013 at 3:37 PM, Stefan Beller\n wrote:\n> When checking the previous lines in that function, we can deduct that\n\ns/deduct/deduce/\n\n> hsize must always be smaller than (1u<<31), since 506049c7df2c6\n> (fix >4GiB source delta assertion failure), because the entries is\n\ns/the entries/entries/ reads a bit better.\n\n> capped at an upper bound of 0xfffffffeU, so hsize contains a maximum\n> value of 0x3fffffff, which is smaller than (1u<<31), so i will never\n\ns/i/it/\n\n> be larger than 31.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff-delta.c | 9 ++++++++-\n> 1 file changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/diff-delta.c b/diff-delta.c\n> index 93385e1..54da95b 100644\n> --- a/diff-delta.c\n> +++ b/diff-delta.c\n> @@ -154,8 +154,15 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)\n> */\n> entries = 0xfffffffeU / RABIN_WINDOW;\n> }\n> +\n> + /*\n> + * Do not check i < 31 in the loop, because the assignement\n> + * previous to the loop makes sure, hsize is definitely\n> + * smaller than 1<<31, hence the loop will always stop\n> + * before i exceeds 31 resulting in an infinite loop.\n> + */\n\nThis comment echoes the commit message, and indeed the explanation\nmakes more sense in that context since someone can read the entire\npatch to see and understand the actual change. It may have less value\nas an in-code comment.\n\n> hsize = entries / 4;\n> - for (i = 4; (1u << i) < hsize && i < 31; i++);\n> + for (i = 4; (1u << i) < hsize; i++);\n> hsize = 1 << i;\n> hmask = hsize - 1;\n>\n> --\n"},{"id":"225288","messageId":"1376602462-32339-1-git-send-email-stefanbeller@googlemail.com","threadId":"34704","inReplyTo":"CAPig+cQmdPo4mo69DsDmUURcw+HbxkAoNEqY08qiuJs8S+=bvQ@mail.gmail.com","subject":"[PATCH] create_delta_index: simplify condition always evaluating to true","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-08-15T21:34:22Z","receivedAt":"2013-08-15T21:34:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When checking the previous lines in that function, we can deduce that\nhsize must always be smaller than (1u<<31), since 506049c7df2c6\n(fix >4GiB source delta assertion failure), because entries is\ncapped at an upper bound of 0xfffffffeU, so hsize contains a maximum\nvalue of 0x3fffffff, which is smaller than (1u<<31), so the value of\n'i' will never be larger than 31.\n\nSigned-off-by: Stefan Beller \n---\n\nEric, thanks for reviewing my patch.\n\nI applied the first 2 proposals (deduce, entries), but I disagree on\nthe third, so I reformulated the sentence, as I really meant the variable\ni and not it as a pronoun.\n\nDo I understand right, you're suggesting to remove the \nsource code comment? I did this now, but I have a bad feeling with it.\n\nThe change of this patch surely removes dead code as of now and makes it\nmore readable. But also it could become alive again, once somebody\nchanges things nearby and forgets about the assumption, hsize not\nexceeding a certain size. That's why I put a comment in there, so \nthe future changes nearby may be more careful.\n\nThanks,\nStefan\n\n\n diff-delta.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff-delta.c b/diff-delta.c\nindex 93385e1..3797ce6 100644\n--- a/diff-delta.c\n+++ b/diff-delta.c\n@@ -155,7 +155,7 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)\n \t\tentries = 0xfffffffeU / RABIN_WINDOW;\n \t}\n \thsize = entries / 4;\n-\tfor (i = 4; (1u << i) < hsize && i < 31; i++);\n+\tfor (i = 4; (1u << i) < hsize; i++);\n \thsize = 1 << i;\n \thmask = hsize - 1;\n \n-- \n1.8.4.rc3.1.gc1ebd90\n"},{"id":"225289","messageId":"7vzjsipr3s.fsf@alter.siamese.dyndns.org","threadId":"34704","inReplyTo":"1376595460-6546-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH] create_delta_index: simplify condition always evaluating to true","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-15T21:43:35Z","receivedAt":"2013-08-15T21:43:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Forwarding to the area expert...\n\nStefan Beller writes:\n\n> When checking the previous lines in that function, we can deduct that\n> hsize must always be smaller than (1u<<31), since 506049c7df2c6\n> (fix >4GiB source delta assertion failure), because the entries is\n> capped at an upper bound of 0xfffffffeU, so hsize contains a maximum\n> value of 0x3fffffff, which is smaller than (1u<<31), so i will never\n> be larger than 31.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff-delta.c | 9 ++++++++-\n> 1 file changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/diff-delta.c b/diff-delta.c\n> index 93385e1..54da95b 100644\n> --- a/diff-delta.c\n> +++ b/diff-delta.c\n> @@ -154,8 +154,15 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)\n> \t\t */\n> \t\tentries = 0xfffffffeU / RABIN_WINDOW;\n> \t}\n> +\n> +\t/*\n> +\t * Do not check i < 31 in the loop, because the assignement\n> +\t * previous to the loop makes sure, hsize is definitely\n> +\t * smaller than 1<<31, hence the loop will always stop\n> +\t * before i exceeds 31 resulting in an infinite loop.\n> +\t */\n> \thsize = entries / 4;\n> -\tfor (i = 4; (1u << i) < hsize && i < 31; i++);\n> +\tfor (i = 4; (1u << i) < hsize; i++);\n> \thsize = 1 << i;\n> \thmask = hsize - 1;\n"},{"id":"225290","messageId":"CAPig+cQ5Y9irLk=9Bhz09c=5yzZEcyMKn2kbhcrO_zDpgmkhGw@mail.gmail.com","threadId":"34704","inReplyTo":"1376602462-32339-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH] create_delta_index: simplify condition always evaluating to true","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-08-15T21:46:54Z","receivedAt":"2013-08-15T21:46:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 15, 2013 at 5:34 PM, Stefan Beller\n wrote:\n> When checking the previous lines in that function, we can deduce that\n> hsize must always be smaller than (1u<<31), since 506049c7df2c6\n> (fix >4GiB source delta assertion failure), because entries is\n> capped at an upper bound of 0xfffffffeU, so hsize contains a maximum\n> value of 0x3fffffff, which is smaller than (1u<<31), so the value of\n> 'i' will never be larger than 31.\n>\n> Signed-off-by: Stefan Beller \n> ---\n>\n> Eric, thanks for reviewing my patch.\n>\n> I applied the first 2 proposals (deduce, entries), but I disagree on\n> the third, so I reformulated the sentence, as I really meant the variable\n> i and not it as a pronoun.\n\nThanks. Adding the quotes around 'i' makes your meaning clear. Without\nthe quotes, apparently it was ambiguous, and my brain read it as a\nmisspelling of 'it'.\n\n> Do I understand right, you're suggesting to remove the\n> source code comment? I did this now, but I have a bad feeling with it.\n>\n> The change of this patch surely removes dead code as of now and makes it\n> more readable. But also it could become alive again, once somebody\n> changes things nearby and forgets about the assumption, hsize not\n> exceeding a certain size. That's why I put a comment in there, so\n> the future changes nearby may be more careful.\n\nIndeed, I feel uncomfortable with the patch in general for the very\nreason that you state: it might become live again. Without the patch,\nthe code remains safe without any extra effort. With this patch, even\nwith the in-code comment, someone making changes needs to take special\ncare. Sometimes it makes sense to leave safeties in place, even if\nthey can't be triggered _today_; and safeties (such as i < 31) also\nserve as documentation.\n\n>\n> Thanks,\n> Stefan\n>\n>\n> diff-delta.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/diff-delta.c b/diff-delta.c\n> index 93385e1..3797ce6 100644\n> --- a/diff-delta.c\n> +++ b/diff-delta.c\n> @@ -155,7 +155,7 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)\n> entries = 0xfffffffeU / RABIN_WINDOW;\n> }\n> hsize = entries / 4;\n> - for (i = 4; (1u << i) < hsize && i < 31; i++);\n> + for (i = 4; (1u << i) < hsize; i++);\n> hsize = 1 << i;\n> hmask = hsize - 1;\n>\n> --\n> 1.8.4.rc3.1.gc1ebd90\n>\n"},{"id":"225291","messageId":"520D5069.5090905@googlemail.com","threadId":"34704","inReplyTo":"7vzjsipr3s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] create_delta_index: simplify condition always evaluating to true","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-08-15T22:04:25Z","receivedAt":"2013-08-15T22:04:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Nicolas,\n\nI am sorry for not including you in the first mail.\nJust before Junio included you, there were these 2 mails\nhttp://www.mail-archive.com/git@vger.kernel.org/msg34101.html\nhttp://www.mail-archive.com/git@vger.kernel.org/msg34103.html\n\nStefan\n\nOn 08/15/2013 11:43 PM, Junio C Hamano wrote:\n> Forwarding to the area expert...\n> \n> Stefan Beller writes:\n> \n>> When checking the previous lines in that function, we can deduct that\n>> hsize must always be smaller than (1u<<31), since 506049c7df2c6\n>> (fix >4GiB source delta assertion failure), because the entries is\n>> capped at an upper bound of 0xfffffffeU, so hsize contains a maximum\n>> value of 0x3fffffff, which is smaller than (1u<<31), so i will never\n>> be larger than 31.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> diff-delta.c | 9 ++++++++-\n>> 1 file changed, 8 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/diff-delta.c b/diff-delta.c\n>> index 93385e1..54da95b 100644\n>> --- a/diff-delta.c\n>> +++ b/diff-delta.c\n>> @@ -154,8 +154,15 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)\n>> \t\t */\n>> \t\tentries = 0xfffffffeU / RABIN_WINDOW;\n>> \t}\n>> +\n>> +\t/*\n>> +\t * Do not check i < 31 in the loop, because the assignement\n>> +\t * previous to the loop makes sure, hsize is definitely\n>> +\t * smaller than 1<<31, hence the loop will always stop\n>> +\t * before i exceeds 31 resulting in an infinite loop.\n>> +\t */\n>> \thsize = entries / 4;\n>> -\tfor (i = 4; (1u << i) < hsize && i < 31; i++);\n>> +\tfor (i = 4; (1u << i) < hsize; i++);\n>> \thsize = 1 << i;\n>> \thmask = hsize - 1;\n\n\n"},{"id":"225292","messageId":"0F8B752CC3F241A9874E8A268A25D162@PhilipOakley","threadId":"34704","inReplyTo":"1376602462-32339-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH] create_delta_index: simplify condition always evaluating to true","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2013-08-15T22:14:23Z","receivedAt":"2013-08-15T22:14:23Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Stefan Beller\" \n> When checking the previous lines in that function, we can deduce that\n> hsize must always be smaller than (1u<<31), since 506049c7df2c6\n> (fix >4GiB source delta assertion failure), because entries is\n> capped at an upper bound of 0xfffffffeU, so hsize contains a maximum\n> value of 0x3fffffff, which is smaller than (1u<<31), so the value of\n> 'i' will never be larger than 31.\n>\n> Signed-off-by: Stefan Beller \n> ---\n>\n> Eric, thanks for reviewing my patch.\n>\n> I applied the first 2 proposals (deduce, entries), but I disagree on\n> the third, so I reformulated the sentence, as I really meant the \n> variable\n> i and not it as a pronoun.\n>\n> Do I understand right, you're suggesting to remove the\n> source code comment? I did this now, but I have a bad feeling with it.\n>\n> The change of this patch surely removes dead code as of now and makes \n> it\n> more readable. But also it could become alive again, once somebody\n> changes things nearby and forgets about the assumption, hsize not\n> exceeding a certain size. That's why I put a comment in there, so\n> the future changes nearby may be more careful.\n\nShould the comment also include a note about potential undefined \nbehaviour of a large shift, with the possible consequential bad compiler \noptimization, such as simply deleting the code. I understand the initial \nspot was part of the STACK tool check of such latent optimisation bugs.\n\nhttp://css.csail.mit.edu/stack/\n\n>\n> Thanks,\n> Stefan\n>\n>\n> diff-delta.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/diff-delta.c b/diff-delta.c\n> index 93385e1..3797ce6 100644\n> --- a/diff-delta.c\n> +++ b/diff-delta.c\n> @@ -155,7 +155,7 @@ struct delta_index * create_delta_index(const void \n> *buf, unsigned long bufsize)\n> entries = 0xfffffffeU / RABIN_WINDOW;\n> }\n> hsize = entries / 4;\n> - for (i = 4; (1u << i) < hsize && i < 31; i++);\n> + for (i = 4; (1u << i) < hsize; i++);\n> hsize = 1 << i;\n> hmask = hsize - 1;\n>\n> -- \n> 1.8.4.rc3.1.gc1ebd90\n>\n"},{"id":"225310","messageId":"alpine.LFD.2.03.1308152231230.14472@syhkavp.arg","threadId":"34704","inReplyTo":"7vzjsipr3s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] create_delta_index: simplify condition always evaluating to true","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2013-08-16T02:34:06Z","receivedAt":"2013-08-16T02:34:06Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 15 Aug 2013, Junio C Hamano wrote:\n\n> Forwarding to the area expert...\n> \n> Stefan Beller writes:\n> \n> > When checking the previous lines in that function, we can deduct that\n> > hsize must always be smaller than (1u<<31), since 506049c7df2c6\n> > (fix >4GiB source delta assertion failure), because the entries is\n> > capped at an upper bound of 0xfffffffeU, so hsize contains a maximum\n> > value of 0x3fffffff, which is smaller than (1u<<31), so i will never\n> > be larger than 31.\n> >\n> > Signed-off-by: Stefan Beller \n\nAcked-by: Nicolas Pitre \n\nYou probably could dispense with the comment. The code is obvious \nenough and the commit log has the rationale already.\n\n> > ---\n> > diff-delta.c | 9 ++++++++-\n> > 1 file changed, 8 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/diff-delta.c b/diff-delta.c\n> > index 93385e1..54da95b 100644\n> > --- a/diff-delta.c\n> > +++ b/diff-delta.c\n> > @@ -154,8 +154,15 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)\n> > \t\t */\n> > \t\tentries = 0xfffffffeU / RABIN_WINDOW;\n> > \t}\n> > +\n> > +\t/*\n> > +\t * Do not check i < 31 in the loop, because the assignement\n> > +\t * previous to the loop makes sure, hsize is definitely\n> > +\t * smaller than 1<<31, hence the loop will always stop\n> > +\t * before i exceeds 31 resulting in an infinite loop.\n> > +\t */\n> > \thsize = entries / 4;\n> > -\tfor (i = 4; (1u << i) < hsize && i < 31; i++);\n> > +\tfor (i = 4; (1u << i) < hsize; i++);\n> > \thsize = 1 << i;\n> > \thmask = hsize - 1;\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"225311","messageId":"alpine.LFD.2.03.1308152236250.14472@syhkavp.arg","threadId":"34704","inReplyTo":"CAPig+cQ5Y9irLk=9Bhz09c=5yzZEcyMKn2kbhcrO_zDpgmkhGw@mail.gmail.com","subject":"Re: [PATCH] create_delta_index: simplify condition always evaluating to true","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2013-08-16T02:38:52Z","receivedAt":"2013-08-16T02:38:52Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 15 Aug 2013, Eric Sunshine wrote:\n\n> On Thu, Aug 15, 2013 at 5:34 PM, Stefan Beller\n> wrote:\n> > When checking the previous lines in that function, we can deduce that\n> > hsize must always be smaller than (1u<<31), since 506049c7df2c6\n> > (fix >4GiB source delta assertion failure), because entries is\n> > capped at an upper bound of 0xfffffffeU, so hsize contains a maximum\n> > value of 0x3fffffff, which is smaller than (1u<<31), so the value of\n> > 'i' will never be larger than 31.\n> >\n> > Signed-off-by: Stefan Beller \n> > ---\n> >\n> > Eric, thanks for reviewing my patch.\n> >\n> > I applied the first 2 proposals (deduce, entries), but I disagree on\n> > the third, so I reformulated the sentence, as I really meant the variable\n> > i and not it as a pronoun.\n> \n> Thanks. Adding the quotes around 'i' makes your meaning clear. Without\n> the quotes, apparently it was ambiguous, and my brain read it as a\n> misspelling of 'it'.\n> \n> > Do I understand right, you're suggesting to remove the\n> > source code comment? I did this now, but I have a bad feeling with it.\n> >\n> > The change of this patch surely removes dead code as of now and makes it\n> > more readable. But also it could become alive again, once somebody\n> > changes things nearby and forgets about the assumption, hsize not\n> > exceeding a certain size. That's why I put a comment in there, so\n> > the future changes nearby may be more careful.\n> \n> Indeed, I feel uncomfortable with the patch in general for the very\n> reason that you state: it might become live again. Without the patch,\n> the code remains safe without any extra effort. With this patch, even\n> with the in-code comment, someone making changes needs to take special\n> care. Sometimes it makes sense to leave safeties in place, even if\n> they can't be triggered _today_; and safeties (such as i < 31) also\n> serve as documentation.\n\nThat's also a valid argument. I don't think this loop is going to \nappear on any trace profile either.\n\nI personally have no strong opinion here.\n\n\nNicolas\n"},{"id":"225334","messageId":"20130816114346.GC5123@vauxhall.crustytoothpaste.net","threadId":"34704","inReplyTo":"1376595460-6546-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH] create_delta_index: simplify condition always evaluating to true","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-08-16T11:43:46Z","receivedAt":"2013-08-16T11:43:46Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Thu, Aug 15, 2013 at 09:37:40PM +0200, Stefan Beller wrote:\n> When checking the previous lines in that function, we can deduct that\n> hsize must always be smaller than (1u<<31), since 506049c7df2c6\n> (fix >4GiB source delta assertion failure), because the entries is\n\nthe entries are\n\n> capped at an upper bound of 0xfffffffeU, so hsize contains a maximum\n> value of 0x3fffffff, which is smaller than (1u<<31), so i will never\n> be larger than 31.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"225335","messageId":"CAPig+cT7KcoVgxnDXUgYuxbpcTkUGqi8m56ArFbYxS5ubftxCw@mail.gmail.com","threadId":"34704","inReplyTo":"20130816114346.GC5123@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH] create_delta_index: simplify condition always evaluating to true","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-08-16T12:40:16Z","receivedAt":"2013-08-16T12:40:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 16, 2013 at 7:43 AM, brian m. carlson\n wrote:\n> On Thu, Aug 15, 2013 at 09:37:40PM +0200, Stefan Beller wrote:\n>> When checking the previous lines in that function, we can deduct that\n>> hsize must always be smaller than (1u<<31), since 506049c7df2c6\n>> (fix >4GiB source delta assertion failure), because the entries is\n>\n> the entries are\n\n'entries' is a variable name, thus singular. Stefan corrected it in v2\nto say \"because entries is capped...\" (though it would have been even\na bit clearer to add quotes around 'entries').\n\n>> capped at an upper bound of 0xfffffffeU, so hsize contains a maximum\n>> value of 0x3fffffff, which is smaller than (1u<<31), so i will never\n>> be larger than 31.\n"},{"id":"225349","messageId":"EE5B338564E14F89B349550B37741AFF@PhilipOakley","threadId":"34704","inReplyTo":"CAPig+cQ5Y9irLk=9Bhz09c=5yzZEcyMKn2kbhcrO_zDpgmkhGw@mail.gmail.com","subject":"Re: [PATCH] create_delta_index: simplify condition always evaluating to true","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2013-08-16T16:47:15Z","receivedAt":"2013-08-16T16:47:15Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Eric Sunshine\" \n> On Thu, Aug 15, 2013 at 5:34 PM, Stefan Beller\n> wrote:\n>> When checking the previous lines in that function, we can deduce that\n>> hsize must always be smaller than (1u<<31), since 506049c7df2c6\n>> (fix >4GiB source delta assertion failure), because entries is\n>> capped at an upper bound of 0xfffffffeU, so hsize contains a maximum\n>> value of 0x3fffffff, which is smaller than (1u<<31), so the value of\n>> 'i' will never be larger than 31.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>>\n>> Eric, thanks for reviewing my patch.\n>>\n>> I applied the first 2 proposals (deduce, entries), but I disagree on\n>> the third, so I reformulated the sentence, as I really meant the \n>> variable\n>> i and not it as a pronoun.\n>\n> Thanks. Adding the quotes around 'i' makes your meaning clear. Without\n> the quotes, apparently it was ambiguous, and my brain read it as a\n> misspelling of 'it'.\n>\n>> Do I understand right, you're suggesting to remove the\n>> source code comment? I did this now, but I have a bad feeling with \n>> it.\n>>\n>> The change of this patch surely removes dead code as of now and makes \n>> it\n>> more readable. But also it could become alive again, once somebody\n>> changes things nearby and forgets about the assumption, hsize not\n>> exceeding a certain size. That's why I put a comment in there, so\n>> the future changes nearby may be more careful.\n>\n> Indeed, I feel uncomfortable with the patch in general for the very\n> reason that you state: it might become live again. Without the patch,\n> the code remains safe without any extra effort.\n\nThe problem is that without the patch (or some change) the code was \nalready unsafe.\n\nThe code sequence ' (1u << i) < hsize && i < 31 ' is a multi step \nprocess, whose first step requires that 'i' is already less that 31, \notherwise the result (1u << i) is undefined (and 'undef_val < hsize' \ncan therefore be assumed to be 'false'), and so the later test i < 31 \ncan always be optimised away as dead code ('i' is already less than 31, \nor the short circuit 'and' applies).\n\nSimply swapping around the code such that the i < 31 test is performed \nfirst would also solve the (latent optimisation) problem.\n\nSection 2.2 of the \"Undefined behavior: What happened to my code?\" paper \non http://css.csail.mit.edu/stack/ discusses this issue with an example \nfrom the Linux kernel.\n\n> With this patch, even\n> with the in-code comment, someone making changes needs to take special\n> care. Sometimes it makes sense to leave safeties in place, even if\n> they can't be triggered _today_; and safeties (such as i < 31) also\n> serve as documentation.\n>\n>>\n>> Thanks,\n>> Stefan\n>>\n>>\n>> diff-delta.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/diff-delta.c b/diff-delta.c\n>> index 93385e1..3797ce6 100644\n>> --- a/diff-delta.c\n>> +++ b/diff-delta.c\n>> @@ -155,7 +155,7 @@ struct delta_index * create_delta_index(const \n>> void *buf, unsigned long bufsize)\n>> entries = 0xfffffffeU / RABIN_WINDOW;\n>> }\n>> hsize = entries / 4;\n>> - for (i = 4; (1u << i) < hsize && i < 31; i++);\n>> + for (i = 4; (1u << i) < hsize; i++);\n>> hsize = 1 << i;\n>> hmask = hsize - 1;\n>>\n>> --\n>> 1.8.4.rc3.1.gc1ebd90\n>>\n> --\n\nPhilip \n"},{"id":"225356","messageId":"1376688157-8374-1-git-send-email-stefanbeller@googlemail.com","threadId":"34704","inReplyTo":"EE5B338564E14F89B349550B37741AFF@PhilipOakley","subject":"[PATCH] create_delta_index: simplify condition always evaluating to true","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-08-16T21:22:37Z","receivedAt":"2013-08-16T21:22:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The code sequence ' (1u << i) < hsize && i < 31 ' is a multi step\nprocess, whose first step requires that 'i' is already less that 31,\notherwise the result (1u << i) is undefined (and 'undef_val < hsize'\ncan therefore be assumed to be 'false'), and so the later test i < 31\ncan always be optimized away as dead code ('i' is already less than 31,\nor the short circuit 'and' applies).\n\nSo we need to get rid of that code. One way would be to exchange the \norder of the conditions, so the expression 'i < 31 && (1u << i) < hsize' \nwould remove that optimized unstable code already.\n\nHowever when checking the previous lines in that function, we can deduce\nthat 'hsize' must always be smaller than (1u<<31), since 506049c7df2c6\n(fix >4GiB source delta assertion failure), because 'entries' is\ncapped at an upper bound of 0xfffffffeU, so 'hsize' contains a maximum\nvalue of 0x3fffffff, which is smaller than (1u<<31), so the value of\n'i' will never be larger than 31 and we can remove that condition \nentirely.\n\nSigned-off-by: Stefan Beller \nAcked-by: Nicolas Pitre \n---\n\nPhilip, \nthanks for the wording of your mail. I get quickly derailed from the\nwarnings of the STACK tool and write the other commit messages than\nI ought to write. I think the wording of your mail nails it pretty\ngood, so I put it in the commit message as well.\n\nStefan\n\n\n diff-delta.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff-delta.c b/diff-delta.c\nindex 93385e1..3797ce6 100644\n--- a/diff-delta.c\n+++ b/diff-delta.c\n@@ -155,7 +155,7 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)\n \t\tentries = 0xfffffffeU / RABIN_WINDOW;\n \t}\n \thsize = entries / 4;\n-\tfor (i = 4; (1u << i) < hsize && i < 31; i++);\n+\tfor (i = 4; (1u << i) < hsize; i++);\n \thsize = 1 << i;\n \thmask = hsize - 1;\n \n-- \n1.8.4.rc3.2.g2c2b664\n"},{"id":"225367","messageId":"E5FB18126E7F42D79E085F3924AC9E07@PhilipOakley","threadId":"34704","inReplyTo":"1376688157-8374-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH] create_delta_index: simplify condition always evaluating to true","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2013-08-17T06:58:39Z","receivedAt":"2013-08-17T06:58:39Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Stefan Beller\" \n> The code sequence ' (1u << i) < hsize && i < 31 ' is a multi step\n> process, whose first step requires that 'i' is already less that 31,\n> otherwise the result (1u << i) is undefined (and 'undef_val < hsize'\n> can therefore be assumed to be 'false'), and so the later test i < 31\n> can always be optimized away as dead code ('i' is already less than \n> 31,\n> or the short circuit 'and' applies).\n>\n> So we need to get rid of that code. One way would be to exchange the\n> order of the conditions, so the expression 'i < 31 && (1u << i) < \n> hsize'\n> would remove that optimized unstable code already.\n>\n> However when checking the previous lines in that function, we can \n> deduce\n> that 'hsize' must always be smaller than (1u<<31), since 506049c7df2c6\n> (fix >4GiB source delta assertion failure), because 'entries' is\n> capped at an upper bound of 0xfffffffeU, so 'hsize' contains a maximum\n> value of 0x3fffffff, which is smaller than (1u<<31), so the value of\n> 'i' will never be larger than 31 and we can remove that condition\n> entirely.\n>\n> Signed-off-by: Stefan Beller \n> Acked-by: Nicolas Pitre \n> ---\n\nAcked-by: Philip Oakley philipoakley@iee.org\n\nIf there are concens that future upgrades may make the (i < 31) test a \nnecessity that it should be added before the &&. If the compiler is able \nto determine it's dead code in that case then that's OK, at least it \nwould be future proof. Then again it's probably not likely in the near \nfuture.\n\n>\n> Philip,\n> thanks for the wording of your mail. I get quickly derailed from the\n> warnings of the STACK tool and write the other commit messages than\n> I ought to write. I think the wording of your mail nails it pretty\n> good, so I put it in the commit message as well.\n>\n> Stefan\n>\n>\n> diff-delta.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/diff-delta.c b/diff-delta.c\n> index 93385e1..3797ce6 100644\n> --- a/diff-delta.c\n> +++ b/diff-delta.c\n> @@ -155,7 +155,7 @@ struct delta_index * create_delta_index(const void \n> *buf, unsigned long bufsize)\n> entries = 0xfffffffeU / RABIN_WINDOW;\n> }\n> hsize = entries / 4;\n> - for (i = 4; (1u << i) < hsize && i < 31; i++);\n> + for (i = 4; (1u << i) < hsize; i++);\n> hsize = 1 << i;\n> hmask = hsize - 1;\n>\n> -- \n> 1.8.4.rc3.2.g2c2b664\n>\n>\nPhilip \n"}]}