From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 02 Jan 2024 16:35:29 GMT
Subject: Re: [Outreachy][PATCH v3] Port helper/test-ctype.c to unit-tests/t-ctype.c
Message-ID: <xmqqbka3g0by.fsf@gitster.g>
In-Reply-To: <435a03c7-dbf3-4ddb-b183-cac86ed0467d@web.de>

René Scharfe <l.s.r@web.de> writes:

>> ...
>> +	for (int i = 0; i < 256; i++) {				\
>> +        	if (!check_int(func(i), ==, is_in(string, i))) 	\
>> +        		test_msg("       i: %02x", i);		\
>> +        } 							\
>
> This loop is indented with spaces followed by tabs.  The Git project
> prefers indenting by tabs and in some cases tabs followed by spaces, but
> not the other way array.  git am warns about such whitespace errors and
> can actually fix them automatically, so I imagine this wouldn't be a
> deal breaker.  But even if it seems picky, respecting the project's
> preferences from the start reduces unnecessary friction.
>
> The original test reported the number of a misclassified character
> (basically its ASCII code) in both decimal and hexadecimal form.  This
> code prints it only in hexadecimal, but without the prefix "0x".  A
> casual reader could mistake hexadecimal numbers for decimal ones as a
> result.  Printing only one suffices, but I think it's better to either
> use decimal notation without any prefix or hexadecimal with the "0x"
> prefix to avoid confusion.  There's no reason to be stingy with the
> screen space here.
> ...
> Each class (e.g. space or digit) is mentioned thrice here: When
> declaring its function with TEST_CTYPE_FUNC, when calling said function
> and again in the test description.  Adding a new class requires adding
> two lines of code.  That's not too bad, but the original implementation
> didn't require that repetition and adding a new class only required
> adding a single line.


Thanks for an excellent review.


>
> I mentioned this briefly in my review of v1 in
> https://lore.kernel.org/git/f743b473-40f8-423d-bf5b-d42b92e5aa1b@web.de/
> and showed a way to define character classes without repeating their
> names.  You don't have to follow that suggestion, but it would be nice
> if you could give some feedback about it.
>
>> +
>> +	return test_done();
>> +}
>
> René