Re: [PATCH] create_delta_index: simplify condition always evaluating to true
From: Stefan Beller stefanbel...@googlemail.com
The code sequence ' (1u i) hsize i 31 ' is a multi step
process, whose first step requires that 'i' is already less that 31,
otherwise the result (1u i) is undefined (and 'undef_val hsize'
can therefore be assumed to be 'false'), and so the later test i 31
can always be optimized away as dead code ('i' is already less than
31,
or the short circuit 'and' applies).
So we need to get rid of that code. One way would be to exchange the
order of the conditions, so the expression 'i 31 (1u i)
hsize'
would remove that optimized unstable code already.
However when checking the previous lines in that function, we can
deduce
that 'hsize' must always be smaller than (1u31), since 506049c7df2c6
(fix 4GiB source delta assertion failure), because 'entries' is
capped at an upper bound of 0xfffeU, so 'hsize' contains a maximum
value of 0x3fff, which is smaller than (1u31), so the value of
'i' will never be larger than 31 and we can remove that condition
entirely.
Signed-off-by: Stefan Beller stefanbel...@googlemail.com
Acked-by: Nicolas Pitre n...@fluxnic.net
---
Acked-by: Philip Oakley philipoak...@iee.org
If there are concens that future upgrades may make the (i 31) test a
necessity that it should be added before the . If the compiler is able
to determine it's dead code in that case then that's OK, at least it
would be future proof. Then again it's probably not likely in the near
future.
Philip,
thanks for the wording of your mail. I get quickly derailed from the
warnings of the STACK tool and write the other commit messages than
I ought to write. I think the wording of your mail nails it pretty
good, so I put it in the commit message as well.
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 = 0xfffeU / 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.2.g2c2b664
Philip
--
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
Re: [PATCH] create_delta_index: simplify condition always evaluating to true
On Thu, Aug 15, 2013 at 09:37:40PM +0200, Stefan Beller wrote: When checking the previous lines in that function, we can deduct that hsize must always be smaller than (1u31), since 506049c7df2c6 (fix 4GiB source delta assertion failure), because the entries is the entries are capped at an upper bound of 0xfffeU, so hsize contains a maximum value of 0x3fff, which is smaller than (1u31), so i will never be larger than 31. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [PATCH] create_delta_index: simplify condition always evaluating to true
On Fri, Aug 16, 2013 at 7:43 AM, brian m. carlson sand...@crustytoothpaste.net wrote: On Thu, Aug 15, 2013 at 09:37:40PM +0200, Stefan Beller wrote: When checking the previous lines in that function, we can deduct that hsize must always be smaller than (1u31), since 506049c7df2c6 (fix 4GiB source delta assertion failure), because the entries is the entries are 'entries' is a variable name, thus singular. Stefan corrected it in v2 to say because entries is capped... (though it would have been even a bit clearer to add quotes around 'entries'). capped at an upper bound of 0xfffeU, so hsize contains a maximum value of 0x3fff, which is smaller than (1u31), so i will never be larger than 31. -- 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
Re: [PATCH] create_delta_index: simplify condition always evaluating to true
From: Eric Sunshine sunsh...@sunshineco.com
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 (1u31), since 506049c7df2c6
(fix 4GiB source delta assertion failure), because entries is
capped at an upper bound of 0xfffeU, so hsize contains a maximum
value of 0x3fff, which is smaller than (1u31), 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.
The problem is that without the patch (or some change) the code was
already unsafe.
The code sequence ' (1u i) hsize i 31 ' is a multi step
process, whose first step requires that 'i' is already less that 31,
otherwise the result (1u i) is undefined (and 'undef_val hsize'
can therefore be assumed to be 'false'), and so the later test i 31
can always be optimised away as dead code ('i' is already less than 31,
or the short circuit 'and' applies).
Simply swapping around the code such that the i 31 test is performed
first would also solve the (latent optimisation) problem.
Section 2.2 of the Undefined behavior: What happened to my code? paper
on http://css.csail.mit.edu/stack/ discusses this issue with an example
from the Linux kernel.
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 = 0xfffeU / 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
--
Philip
--
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
[PATCH] create_delta_index: simplify condition always evaluating to true
The code sequence ' (1u i) hsize i 31 ' is a multi step
process, whose first step requires that 'i' is already less that 31,
otherwise the result (1u i) is undefined (and 'undef_val hsize'
can therefore be assumed to be 'false'), and so the later test i 31
can always be optimized away as dead code ('i' is already less than 31,
or the short circuit 'and' applies).
So we need to get rid of that code. One way would be to exchange the
order of the conditions, so the expression 'i 31 (1u i) hsize'
would remove that optimized unstable code already.
However when checking the previous lines in that function, we can deduce
that 'hsize' must always be smaller than (1u31), since 506049c7df2c6
(fix 4GiB source delta assertion failure), because 'entries' is
capped at an upper bound of 0xfffeU, so 'hsize' contains a maximum
value of 0x3fff, which is smaller than (1u31), so the value of
'i' will never be larger than 31 and we can remove that condition
entirely.
Signed-off-by: Stefan Beller stefanbel...@googlemail.com
Acked-by: Nicolas Pitre n...@fluxnic.net
---
Philip,
thanks for the wording of your mail. I get quickly derailed from the
warnings of the STACK tool and write the other commit messages than
I ought to write. I think the wording of your mail nails it pretty
good, so I put it in the commit message as well.
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 = 0xfffeU / 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.2.g2c2b664
--
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
[PATCH] create_delta_index: simplify condition always evaluating to true
When checking the previous lines in that function, we can deduct that hsize must always be smaller than (1u31), since 506049c7df2c6 (fix 4GiB source delta assertion failure), because the entries is capped at an upper bound of 0xfffeU, so hsize contains a maximum value of 0x3fff, which is smaller than (1u31), so i will never be larger than 31. Signed-off-by: Stefan Beller stefanbel...@googlemail.com --- diff-delta.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/diff-delta.c b/diff-delta.c index 93385e1..54da95b 100644 --- a/diff-delta.c +++ b/diff-delta.c @@ -154,8 +154,15 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize) */ entries = 0xfffeU / RABIN_WINDOW; } + + /* +* Do not check i 31 in the loop, because the assignement +* previous to the loop makes sure, hsize is definitely +* smaller than 131, hence the loop will always stop +* before i exceeds 31 resulting in an infinite loop. +*/ 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.498.g5af1768 -- 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
Re: [PATCH] create_delta_index: simplify condition always evaluating to true
On Thu, Aug 15, 2013 at 3:37 PM, Stefan Beller stefanbel...@googlemail.com wrote: When checking the previous lines in that function, we can deduct that s/deduct/deduce/ hsize must always be smaller than (1u31), since 506049c7df2c6 (fix 4GiB source delta assertion failure), because the entries is s/the entries/entries/ reads a bit better. capped at an upper bound of 0xfffeU, so hsize contains a maximum value of 0x3fff, which is smaller than (1u31), so i will never s/i/it/ be larger than 31. Signed-off-by: Stefan Beller stefanbel...@googlemail.com --- diff-delta.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/diff-delta.c b/diff-delta.c index 93385e1..54da95b 100644 --- a/diff-delta.c +++ b/diff-delta.c @@ -154,8 +154,15 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize) */ entries = 0xfffeU / RABIN_WINDOW; } + + /* +* Do not check i 31 in the loop, because the assignement +* previous to the loop makes sure, hsize is definitely +* smaller than 131, hence the loop will always stop +* before i exceeds 31 resulting in an infinite loop. +*/ This comment echoes the commit message, and indeed the explanation makes more sense in that context since someone can read the entire patch to see and understand the actual change. It may have less value as an in-code comment. 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; -- -- 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
[PATCH] create_delta_index: simplify condition always evaluating to true
When checking the previous lines in that function, we can deduce that hsize must always be smaller than (1u31), since 506049c7df2c6 (fix 4GiB source delta assertion failure), because entries is capped at an upper bound of 0xfffeU, so hsize contains a maximum value of 0x3fff, which is smaller than (1u31), 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. 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. 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 = 0xfffeU / 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
Re: [PATCH] create_delta_index: simplify condition always evaluating to true
Forwarding to the area expert... Stefan Beller stefanbel...@googlemail.com writes: When checking the previous lines in that function, we can deduct that hsize must always be smaller than (1u31), since 506049c7df2c6 (fix 4GiB source delta assertion failure), because the entries is capped at an upper bound of 0xfffeU, so hsize contains a maximum value of 0x3fff, which is smaller than (1u31), so i will never be larger than 31. Signed-off-by: Stefan Beller stefanbel...@googlemail.com --- diff-delta.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/diff-delta.c b/diff-delta.c index 93385e1..54da95b 100644 --- a/diff-delta.c +++ b/diff-delta.c @@ -154,8 +154,15 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize) */ entries = 0xfffeU / RABIN_WINDOW; } + + /* + * Do not check i 31 in the loop, because the assignement + * previous to the loop makes sure, hsize is definitely + * smaller than 131, hence the loop will always stop + * before i exceeds 31 resulting in an infinite loop. + */ 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; -- 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
Re: [PATCH] create_delta_index: simplify condition always evaluating to true
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 (1u31), since 506049c7df2c6 (fix 4GiB source delta assertion failure), because entries is capped at an upper bound of 0xfffeU, so hsize contains a maximum value of 0x3fff, which is smaller than (1u31), 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 = 0xfffeU / 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
Re: [PATCH] create_delta_index: simplify condition always evaluating to true
Nicolas, I am sorry for not including you in the first mail. Just before Junio included you, there were these 2 mails http://www.mail-archive.com/git@vger.kernel.org/msg34101.html http://www.mail-archive.com/git@vger.kernel.org/msg34103.html Stefan On 08/15/2013 11:43 PM, Junio C Hamano wrote: Forwarding to the area expert... Stefan Beller stefanbel...@googlemail.com writes: When checking the previous lines in that function, we can deduct that hsize must always be smaller than (1u31), since 506049c7df2c6 (fix 4GiB source delta assertion failure), because the entries is capped at an upper bound of 0xfffeU, so hsize contains a maximum value of 0x3fff, which is smaller than (1u31), so i will never be larger than 31. Signed-off-by: Stefan Beller stefanbel...@googlemail.com --- diff-delta.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/diff-delta.c b/diff-delta.c index 93385e1..54da95b 100644 --- a/diff-delta.c +++ b/diff-delta.c @@ -154,8 +154,15 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize) */ entries = 0xfffeU / RABIN_WINDOW; } + +/* + * Do not check i 31 in the loop, because the assignement + * previous to the loop makes sure, hsize is definitely + * smaller than 131, hence the loop will always stop + * before i exceeds 31 resulting in an infinite loop. + */ 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; signature.asc Description: OpenPGP digital signature
Re: [PATCH] create_delta_index: simplify condition always evaluating to true
From: Stefan Beller stefanbel...@googlemail.com When checking the previous lines in that function, we can deduce that hsize must always be smaller than (1u31), since 506049c7df2c6 (fix 4GiB source delta assertion failure), because entries is capped at an upper bound of 0xfffeU, so hsize contains a maximum value of 0x3fff, which is smaller than (1u31), 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. 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 = 0xfffeU / 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
Re: [PATCH] create_delta_index: simplify condition always evaluating to true
On Thu, 15 Aug 2013, Junio C Hamano wrote: Forwarding to the area expert... Stefan Beller stefanbel...@googlemail.com writes: When checking the previous lines in that function, we can deduct that hsize must always be smaller than (1u31), since 506049c7df2c6 (fix 4GiB source delta assertion failure), because the entries is capped at an upper bound of 0xfffeU, so hsize contains a maximum value of 0x3fff, which is smaller than (1u31), so i will never be larger than 31. Signed-off-by: Stefan Beller stefanbel...@googlemail.com Acked-by: Nicolas Pitre n...@fluxnic.net You probably could dispense with the comment. The code is obvious enough and the commit log has the rationale already. --- diff-delta.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/diff-delta.c b/diff-delta.c index 93385e1..54da95b 100644 --- a/diff-delta.c +++ b/diff-delta.c @@ -154,8 +154,15 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize) */ entries = 0xfffeU / RABIN_WINDOW; } + + /* +* Do not check i 31 in the loop, because the assignement +* previous to the loop makes sure, hsize is definitely +* smaller than 131, hence the loop will always stop +* before i exceeds 31 resulting in an infinite loop. +*/ 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; -- 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 -- 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
Re: [PATCH] create_delta_index: simplify condition always evaluating to true
On Thu, 15 Aug 2013, Eric Sunshine wrote: 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 (1u31), since 506049c7df2c6 (fix 4GiB source delta assertion failure), because entries is capped at an upper bound of 0xfffeU, so hsize contains a maximum value of 0x3fff, which is smaller than (1u31), 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. That's also a valid argument. I don't think this loop is going to appear on any trace profile either. I personally have no strong opinion here. Nicolas -- 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
