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

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

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.

>> +#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.

>> +	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.