Re: [PATCH GSoC RFC v12 03/12] cat-file: add declaration of variable i inside its for loop

[Junio C Hamano <gitster@pobox.com>]

Pablo Sabater <pabloosabaterr@gmail.com> writes:

> From: Eric Ju <eric.peijian@gmail.com>
> Subject: Re: [PATCH GSoC RFC v12 03/12] cat-file: add declaration of variable i inside its for loop

"add" sounds a bit strange, as the existing code wouldn't have
compiled if the variable were never declared.  What the patch did
was to move (not add) the declaration of a function scope variable
that is used to control for() loops.  Would any of these work?

Subject: [PATCH GSOC v12 03/12] cat-file: narrow scope of loop counter
Subject: [PATCH GSOC v12 03/12] cat-file: declare loop counter inside for()

> Some code used in this series declares variable i and only uses it
> in a for loop, not in any other logic outside the loop.
>
> Change the declaration of i to be inside the for loop for readability.
> While at it, we also change its type from "int" to "size_t" where the latter makes more sense.

Curious single line that is overly long?

> Helped-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Eric Ju <eric.peijian@gmail.com>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
>  builtin/cat-file.c | 11 +++--------
>  fetch-pack.c       |  3 +--
>  2 files changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index fa45f774d7..c060fd4800 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -726,12 +726,10 @@ static void dispatch_calls(struct batch_options *opt,
>  		struct queued_cmd *cmd,
>  		int nr)
>  {
> -	int i;
> -
>  	if (!opt->buffer_output)
>  		die(_("flush is only for --buffer mode"));
>  
> -	for (i = 0; i < nr; i++)
> +	for (size_t i = 0; i < nr; i++)
>  		cmd[i].fn(opt, cmd[i].line, output, data);

The loop limit "nr" will not become as large as size_t because the
caller passes a platform natural "int" to the function.  Wouldn't a
stupid compiler give us warning on comparing unsigned size_t with
signed int here?

> @@ -739,9 +737,7 @@ static void dispatch_calls(struct batch_options *opt,
>  
>  static void free_cmds(struct queued_cmd *cmd, size_t *nr)
>  {
> -	size_t i;
> -
> -	for (i = 0; i < *nr; i++)
> +	for (size_t i = 0; i < *nr; i++)
>  		FREE_AND_NULL(cmd[i].line);

No type change, so the result is as safe as the original.

> @@ -768,7 +764,6 @@ static void batch_objects_command(struct batch_options *opt,
>  	size_t alloc = 0, nr = 0;
>  
>  	while (strbuf_getdelim_strip_crlf(&input, stdin, opt->input_delim) != EOF) {
> -		int i;
>  		const struct parse_cmd *cmd = NULL;
>  		const char *p = NULL, *cmd_end;
>  		struct queued_cmd call = {0};
> @@ -778,7 +773,7 @@ static void batch_objects_command(struct batch_options *opt,
>  		if (isspace(*input.buf))
>  			die(_("whitespace before command: '%s'"), input.buf);
>  
> -		for (i = 0; i < ARRAY_SIZE(commands); i++) {
> +		for (size_t i = 0; i < ARRAY_SIZE(commands); i++) {
>  			if (!skip_prefix(input.buf, commands[i].name, &cmd_end))
>  				continue;

ARRAY_SIZE() is some arithmetic over sizeof(*commands) and
sizeof(commands), which is of type size_t, so this is better than
the original.  Use of size_t i of course is a natural way to index
into commands[] array, so the result is just fine.

> diff --git a/fetch-pack.c b/fetch-pack.c
> index 120e01f3cf..f13951d154 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1388,9 +1388,8 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
>  	if (advertise_sid && server_supports_v2("session-id"))
>  		packet_buf_write(req_buf, "session-id=%s", trace2_session_id());
>  	if (server_options && server_options->nr) {
> -		int i;
>  		ensure_server_supports_v2("server-option");
> -		for (i = 0; i < server_options->nr; i++)
> +		for (size_t i = 0; i < server_options->nr; i++)
>  			packet_buf_write(req_buf, "server-option=%s",
>  					 server_options->items[i].string);

server_options is a string_list whose .nr member is of type size_t,
so this comparison is perfectly fine.  Ditto for ->items[i].string
that is a natural way to index into an array.

>  	}

v