for loop variable fixes
I've been working toward cleaning up the C integer type use from various
angles (signed/unsigned mismatches, int/size_t misuse, etc.). More
about that to come. Here is a small subset to get started.
I have noticed that many for loops have loop variables whose type
doesn't match the upper-bound condition. Meaning, if you have
for (i = 0; i < foo; i++)
then the type of i should ordinarily be the same as the type of foo. At
least if you declare i specifically for that purpose, you might as well
make it match. However, as you can see in the attached patches, lots of
randomness abounds.
As far as I can tell, none of of these are current bugs, but this is the
kind of thing that sets bad precedents and could lead to truncated
values or unintended overflows etc. in the future.
I have located these sites with gcc -Wsign-compare, so that means that
there could be other issues of this nature that are not caught by that
option because the signedness does match but maybe the underlying types
do not. I have started to investigate these too (e.g. gcc
-Wconversion), but this patch is already sizable enough, so I'm going
with this for now.
Another thing to note is that -Wsign-compare is included in gcc -Wall in
C++. So with the increasing interest in C++ for extensions, this kind
of hygiene would also be beneficial.
(But just for the avoidance of doubt: This does not claim to fix all
-Wsign-compare warnings.)
I split this patch into two mainly because the second patch is entirely
repetitive, but they both address the same topic.
Peter Eisentraut <peter@eisentraut.org> writes:
I have noticed that many for loops have loop variables whose type
doesn't match the upper-bound condition. Meaning, if you have
for (i = 0; i < foo; i++)
then the type of i should ordinarily be the same as the type of foo.
+1 for improving this --- I was annoyed about it just yesterday
with respect to resowner.c. It looks to me like just about every
single instance of "for (int ..." in that file is wrong because
what's being compared to is unsigned. I see that your patch gets
a couple of those places, but only a couple, which makes me wonder
what -Wsign-compare is checking exactly.
As far as I can tell, none of of these are current bugs, but this is the
kind of thing that sets bad precedents and could lead to truncated
values or unintended overflows etc. in the future.
Right. For resowner.c, I don't think the compared-to values can
actually reach INT_MAX, but that doesn't make this good code.
regards, tom lane
On 23.06.26 16:12, Tom Lane wrote:
+1 for improving this --- I was annoyed about it just yesterday
with respect to resowner.c. It looks to me like just about every
single instance of "for (int ..." in that file is wrong because
what's being compared to is unsigned. I see that your patch gets
a couple of those places, but only a couple, which makes me wonder
what -Wsign-compare is checking exactly.
I see several instances like
for (int idx = 0; idx < owner->narr; idx++)
where narr is of type uint8. Since uint8 is of lower rank than int, it
gets converted to int for the comparison operator, and then there is no
more type mismatch. So that would appear to be why the warning doesn't
trigger for these.