Re: [PATCH v4 2/2] t/: migrate helper/test-{sha1, sha256} to unit-tests/t-hash

From: Junio C Hamano <gitster@pobox.com>
Date: May 29, 2024, 14:54 UTC
Message-ID: <xmqqed9k1xfa.fsf@gitster.g>
In-Reply-To: <ZlbKkMfmWFw59aO8@tanuki>

Patrick Steinhardt <ps@pks.im> writes:

>> +	for (int i = 1; i < ARRAY_SIZE(hash_algos); i++) {
>
> s/int/size_t/

If ARRAY_SIZE(hash_algos) is an unbounded quantity that is externally controlled, this does make very much sense, but for hash_algos[]? It is not worth the patch noise to go and fix it.

Show 5 quoted lines

>> +#define TEST_HASH_STR(data, expected_sha1, expected_sha256) \
>> +	{ \
>
> These macros should like start with `do {`. The reason why we do this is
> that the compiler will complain if there is no semicolon after the macro.

The idiom is

	#define foo(a,b,c) do { \
		...; \
	} while (0)

so that you can write

	foo(1,2,3);

as if it is a regular function call terminated _with_ a semicolon. Also this allows us to say

	if (condition)
		foo(1,2,3);
	else
		foo(4,5,6);

which would break if the definition were a mere

	#define foo(x,y,z) { \
		... \
	}

in which case ";" after the first foo() terminates the "if" statement and "else" triggers a syntax error.

Show 14 quoted lines

>> +	TEST_HASH_STR(
>> +		"", "da39a3ee5e6b4b0d3255bfef95601890afd80709",
>> +		"e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855");
>
> I think these might've been a bit easier to read if they formatted like
> this:
>
> 	TEST_HASH_STR("",
> 	    "da39a3ee5e6b4b0d3255bfef95601890afd80709",
> 	    "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855");
>
> 	TEST_HASH_STR("a",
> 	    "86f7e437faa5a7fce15d1ddcb9eaeaea377667b8",
> 	    "ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb");

Oh, absolutely.