On Fri, Nov 29, 2024 at 02:13:29PM +0100, Patrick Steinhardt wrote:
> We have a bunch of loops all over the place which iterate through a
> signed integer with an unsigned index, which generates warnings. Address
I had trouble parsing this first sentence. Did you mean "max" or "bound"
instead of "index"? I'd have also thought that "with" is the iteration
variable, not the boundary. So:
...which iterate up to an unsigned boundary using a signed index (and
thus comparing the signed and unsigned values in the loop condition).
or something?
> @@ -195,9 +192,7 @@ int git_default_advice_config(const char *var, const char *value)
>
> void list_config_advices(struct string_list *list, const char *prefix)
> {
> - int i;
> -
> - for (i = 0; i < ARRAY_SIZE(advice_setting); i++)
> + for (size_t i = 0; i < ARRAY_SIZE(advice_setting); i++)
> list_config_item(list, prefix, advice_setting[i].key);
> }
>
I didn't exhaustively stare at each one, but skimmed through and they
all look like obvious improvements, like this.
I do note that you tend to move the declaration of the variable to scope
it to the loop. I think that's OK (and in general is a good practice),
but two thoughts:
1. Are we all-in on C99 loop scoping like this? I wasn't sure if we
were still in the weather-balloon period (but I admit I have not
paid close attention, so this is really just double-checking).
2. The compiler should mostly catch any cases where this is not
correct to do (because any references to the variable outside the
loop would now be errors). The exception is if "i" here were
shadowing another variable. That seems pretty unlikely, though.
-Peff
