Re: [PATCH v3 2/7] t: harmonize t-reftable-merged.c with coding guidelines

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

Chandra Pratap <chandrapratap3519@gmail.com> writes:

It is very nice that the steps [1/7] and [2/7] are split this way.

> Harmonize the newly ported test unit-tests/t-reftable-merged.c
> with the following guidelines:
> - Single line control flow statements like 'for' and 'if'
>   must omit curly braces.

OK.

> - Structs must be 0-initialized with '= { 0 }' instead of '= { NULL }'.

Correct.

> - Array indices must be of type 'size_t', not 'int'.

OK, but "must be" is probably a bit too strong (see below).

> - It is fine to use C99 initial declaration in 'for' loop.

Yes, it is fine.  You do not have to, but you can.

> @@ -68,7 +66,6 @@ static void write_test_log_table(struct strbuf *buf,
>  				 struct reftable_log_record logs[], int n,
>  				 uint64_t update_index)
>  {
> -	int i = 0;
>  	int err;
>  
>  	struct reftable_write_options opts = {
> @@ -79,7 +76,7 @@ static void write_test_log_table(struct strbuf *buf,
>  	w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
>  	reftable_writer_set_limits(w, update_index, update_index);
>  
> -	for (i = 0; i < n; i++) {
> +	for (int i = 0; i < n; i++) {
>  		int err = reftable_writer_add_log(w, &logs[i]);

Did you mean size_t instead of int here?  Probably not, because the
iteration goes up to "int n" that is supplied by the caller of this
test, so iterating with "int" is perfectly fine here.

So, "must be size_t" is already violated here.  You could update the
type of the incoming parameter "n", but given that this is a test
program that deals with a known logs[] array of a small bounded
size, that may be way overkill and "int" can be justified, too.
On the other hand, if it does not require too much investigation,
you may want to check the caller and if it can be updated to use
"size_t" instead of "int".

The general rule is probably "think twice before using 'int' as an
array index; otherwise use 'size_t'", which covers what I said in
the above paragraph.

> @@ -121,8 +118,7 @@ merged_table_from_records(struct reftable_ref_record **refs,
>  
>  static void readers_destroy(struct reftable_reader **readers, size_t n)
>  {
> -	int i = 0;
> -	for (; i < n; i++)
> +	for (size_t i = 0; i < n; i++)
>  		reftable_reader_free(readers[i]);
>  	reftable_free(readers);
>  }

Much better.

> @@ -148,9 +144,8 @@ static void t_merged_single_record(void)
>  	struct reftable_reader **readers = NULL;
>  	struct reftable_merged_table *mt =
>  		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 2);
> -	int i;
> -	struct reftable_ref_record ref = { NULL };
> -	struct reftable_iterator it = { NULL };
> +	struct reftable_ref_record ref = { 0 };
> +	struct reftable_iterator it = { 0 };
>  	int err;
>  
>  	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
> @@ -164,9 +159,8 @@ static void t_merged_single_record(void)
>  	reftable_iterator_destroy(&it);
>  	readers_destroy(readers, 2);
>  	reftable_merged_table_free(mt);
> -	for (i = 0; i < ARRAY_SIZE(bufs); i++) {
> +	for (size_t i = 0; i < ARRAY_SIZE(bufs); i++)
>  		strbuf_release(&bufs[i]);
> -	}

OK.  size_t is overkill here because bufs[] is a function local
array with only two elements in it, but once the patch to use
"size_t" (i.e., this one) is written, it is not worth to go in and
make it use "int" again.

> @@ -226,12 +220,12 @@ static void t_merged_refs(void)
>  	struct reftable_reader **readers = NULL;
>  	struct reftable_merged_table *mt =
>  		merged_table_from_records(refs, &bs, &readers, sizes, bufs, 3);
> -	struct reftable_iterator it = { NULL };
> +	struct reftable_iterator it = { 0 };
>  	int err;
>  	struct reftable_ref_record *out = NULL;
>  	size_t len = 0;
>  	size_t cap = 0;
> -	int i = 0;
> +	size_t i;

OK.  It is good that we got rid of useless initialization, as this
is used to drive more than one loops below.

> @@ -358,12 +349,12 @@ static void t_merged_logs(void)
>  	struct reftable_reader **readers = NULL;
>  	struct reftable_merged_table *mt = merged_table_from_log_records(
>  		logs, &bs, &readers, sizes, bufs, 3);
> -	struct reftable_iterator it = { NULL };
> +	struct reftable_iterator it = { 0 };
>  	int err;
>  	struct reftable_log_record *out = NULL;
>  	size_t len = 0;
>  	size_t cap = 0;
> -	int i = 0;
> +	size_t i = 0;

Lose the useless initialization here, too.