Re: [PATCH v3 2/7] t: harmonize t-reftable-merged.c with coding guidelines
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 11, 2024, 20:38 UTC
- Message-ID
- <xmqq7cdrr7f2.fsf@gitster.g>
- In-Reply-To
- <20240711040854.4602-3-chandrapratap3519@gmail.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.
Show 15 quoted lines
> @@ -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.
Show 10 quoted lines
> @@ -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.
Show 20 quoted lines
> @@ -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.
Show 12 quoted lines
> @@ -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.
Show 12 quoted lines
> @@ -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.