diff mbox series

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

Message ID 20240711040854.4602-3-chandrapratap3519@gmail.com (mailing list archive)
State Superseded
Headers show
Series t: port reftable/merged_test.c to the unit testing framework | expand

Commit Message

Chandra Pratap July 11, 2024, 3:58 a.m. UTC 
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.
- Structs must be 0-initialized with '= { 0 }' instead of '= { NULL }'.
- Array indices must be of type 'size_t', not 'int'.
- It is fine to use C99 initial declaration in 'for' loop.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-merged.c | 52 ++++++++++++--------------------
 1 file changed, 20 insertions(+), 32 deletions(-)

Comments

Junio C Hamano July 11, 2024, 8:38 p.m. UTC | #1 
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.
diff mbox series

Patch

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index 78a864a54f..a984116619 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -40,12 +40,10 @@  static void write_test_table(struct strbuf *buf,
 	struct reftable_writer *w = NULL;
 	for (i = 0; i < n; i++) {
 		uint64_t ui = refs[i].update_index;
-		if (ui > max) {
+		if (ui > max)
 			max = ui;
-		}
-		if (ui < min) {
+		if (ui < min)
 			min = ui;
-		}
 	}
 
 	w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
@@ -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]);
 		check(!err);
 	}
@@ -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);
 }
@@ -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]);
-	}
 	reftable_free(bs);
 }
 
@@ -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;
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
 	err = reftable_iterator_seek_ref(&it, "a");
@@ -240,7 +234,7 @@  static void t_merged_refs(void)
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
-		struct reftable_ref_record ref = { NULL };
+		struct reftable_ref_record ref = { 0 };
 		int err = reftable_iterator_next_ref(&it, &ref);
 		if (err > 0)
 			break;
@@ -251,18 +245,15 @@  static void t_merged_refs(void)
 	reftable_iterator_destroy(&it);
 
 	check_int(ARRAY_SIZE(want), ==, len);
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		check(reftable_ref_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
-	}
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		reftable_ref_record_release(&out[i]);
-	}
 	reftable_free(out);
 
-	for (i = 0; i < 3; i++) {
+	for (i = 0; i < 3; i++)
 		strbuf_release(&bufs[i]);
-	}
 	readers_destroy(readers, 3);
 	reftable_merged_table_free(mt);
 	reftable_free(bs);
@@ -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;
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log(&it, "a");
@@ -372,7 +363,7 @@  static void t_merged_logs(void)
 	check_int(reftable_merged_table_min_update_index(mt), ==, 1);
 
 	while (len < 100) { /* cap loops/recursion. */
-		struct reftable_log_record log = { NULL };
+		struct reftable_log_record log = { 0 };
 		int err = reftable_iterator_next_log(&it, &log);
 		if (err > 0)
 			break;
@@ -383,10 +374,9 @@  static void t_merged_logs(void)
 	reftable_iterator_destroy(&it);
 
 	check_int(ARRAY_SIZE(want), ==, len);
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		check(reftable_log_record_equal(want[i], &out[i],
 						 GIT_SHA1_RAWSZ));
-	}
 
 	merged_table_init_iter(mt, &it, BLOCK_TYPE_LOG);
 	err = reftable_iterator_seek_log_at(&it, "a", 2);
@@ -397,14 +387,12 @@  static void t_merged_logs(void)
 	check(reftable_log_record_equal(&out[0], &r3[0], GIT_SHA1_RAWSZ));
 	reftable_iterator_destroy(&it);
 
-	for (i = 0; i < len; i++) {
+	for (i = 0; i < len; i++)
 		reftable_log_record_release(&out[i]);
-	}
 	reftable_free(out);
 
-	for (i = 0; i < 3; i++) {
+	for (i = 0; i < 3; i++)
 		strbuf_release(&bufs[i]);
-	}
 	readers_destroy(readers, 3);
 	reftable_merged_table_free(mt);
 	reftable_free(bs);
@@ -422,7 +410,7 @@  static void t_default_write_opts(void)
 		.update_index = 1,
 	};
 	int err;
-	struct reftable_block_source source = { NULL };
+	struct reftable_block_source source = { 0 };
 	struct reftable_table *tab = reftable_calloc(1, sizeof(*tab));
 	uint32_t hash_id;
 	struct reftable_reader *rd = NULL;