René Scharfe <l.s.r@xxxxxx> 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@xxxxxx/
> 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é
