{"thread":{"id":"60991","subject":"[PATCH 0/3] t-ctype: simplify unit test definitions","startedAt":"2024-02-25T11:27:34Z","lastAt":"2024-03-10T12:48:35Z","messageCount":30,"participants":["René Scharfe","Eric Sunshine","Jeff King","Christian Couder","Junio C Hamano","Josh Steadmon","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"489303","messageId":"20240225112722.89221-1-l.s.r@web.de","threadId":"60991","inReplyTo":null,"subject":"[PATCH 0/3] t-ctype: simplify unit test definitions","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-25T11:27:17Z","receivedAt":"2024-02-25T11:27:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Simplify the ctype unit tests to allow combining specification strings\nin any order and no longer require repeating class names.  Patch 3 gets\nrid of aggregation by class and implements the tests in arguably the\nmost basic and straight-forward way for the unit test framework, at the\ncost of producing raw and lengthy output.\n\n  t-ctype: allow NUL anywhere in the specification string\n  t-ctype: avoid duplicating class names\n  t-ctype: do one test per class and char\n\n t/unit-tests/t-ctype.c | 73 ++++++++++++------------------------------\n 1 file changed, 20 insertions(+), 53 deletions(-)\n\n--\n2.44.0\n\n"},{"id":"489304","messageId":"20240225112722.89221-2-l.s.r@web.de","threadId":"60991","inReplyTo":"20240225112722.89221-1-l.s.r@web.de","subject":"[PATCH 1/3] t-ctype: allow NUL anywhere in the specification string","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-25T11:27:18Z","receivedAt":"2024-02-25T11:27:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Replace the custom function is_in() for looking up a character in the\nspecification string with memchr(3) and sizeof.  This is shorter,\nsimpler and allows NUL anywhere in the string, which may come in handy\nif we ever want to support more character classes that contain it.\n\nGetting the string size using sizeof only works in a macro and with a\nstring constant, but that's exactly what we have and I don't see it\nchanging anytime soon.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/unit-tests/t-ctype.c | 16 ++--------------\n 1 file changed, 2 insertions(+), 14 deletions(-)\n\ndiff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\nindex f315489984..64d7186258 100644\n--- a/t/unit-tests/t-ctype.c\n+++ b/t/unit-tests/t-ctype.c\n@@ -1,23 +1,11 @@\n #include \"test-lib.h\"\n\n-static int is_in(const char *s, int ch)\n-{\n-\t/*\n-\t * We can't find NUL using strchr. Accept it as the first\n-\t * character in the spec -- there are no empty classes.\n-\t */\n-\tif (ch == '\\0')\n-\t\treturn ch == *s;\n-\tif (*s == '\\0')\n-\t\ts++;\n-\treturn !!strchr(s, ch);\n-}\n-\n /* Macro to test a character type */\n #define TEST_CTYPE_FUNC(func, string) \\\n static void test_ctype_##func(void) { \\\n \tfor (int i = 0; i < 256; i++) { \\\n-\t\tif (!check_int(func(i), ==, is_in(string, i))) \\\n+\t\tint expect = !!memchr(string, i, sizeof(string) - 1); \\\n+\t\tif (!check_int(func(i), ==, expect)) \\\n \t\t\ttest_msg(\"       i: 0x%02x\", i); \\\n \t} \\\n \tif (!check(!func(EOF))) \\\n--\n2.44.0\n\n"},{"id":"489305","messageId":"20240225112722.89221-3-l.s.r@web.de","threadId":"60991","inReplyTo":"20240225112722.89221-1-l.s.r@web.de","subject":"[PATCH 2/3] t-ctype: avoid duplicating class names","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-25T11:27:19Z","receivedAt":"2024-02-25T11:27:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"TEST_CTYPE_FUNC defines a function for testing a character classifier,\nTEST_CHAR_CLASS calls it, causing the class name to be mentioned twice.\n\nAvoid the need to define a class-specific function by letting\nTEST_CHAR_CLASS do all the work.  This is done by using the internal\nfunctions test__run_begin() and test__run_end(), but they do exist to be\nused in test macros after all.\n\nAlternatively we could unroll the loop to provide a very long expression\nthat tests all 256 characters and EOF and hand that to TEST, but that\nseems awkward and hard to read.\n\nNo change of behavior or output intended.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/unit-tests/t-ctype.c | 68 ++++++++++++++++--------------------------\n 1 file changed, 26 insertions(+), 42 deletions(-)\n\ndiff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\nindex 64d7186258..56dfefb68e 100644\n--- a/t/unit-tests/t-ctype.c\n+++ b/t/unit-tests/t-ctype.c\n@@ -1,18 +1,18 @@\n #include \"test-lib.h\"\n\n-/* Macro to test a character type */\n-#define TEST_CTYPE_FUNC(func, string) \\\n-static void test_ctype_##func(void) { \\\n-\tfor (int i = 0; i < 256; i++) { \\\n-\t\tint expect = !!memchr(string, i, sizeof(string) - 1); \\\n-\t\tif (!check_int(func(i), ==, expect)) \\\n-\t\t\ttest_msg(\"       i: 0x%02x\", i); \\\n-\t} \\\n-\tif (!check(!func(EOF))) \\\n+#define TEST_CHAR_CLASS(class, string) do { \\\n+\tint skip = test__run_begin(); \\\n+\tif (!skip) { \\\n+\t\tfor (int i = 0; i < 256; i++) { \\\n+\t\t\tint expect = !!memchr(string, i, sizeof(string) - 1); \\\n+\t\t\tif (!check_int(class(i), ==, expect)) \\\n+\t\t\t\ttest_msg(\"       i: 0x%02x\", i); \\\n+\t\t} \\\n+\t\tif (!check(!class(EOF))) \\\n \t\t\ttest_msg(\"      i: 0x%02x (EOF)\", EOF); \\\n-}\n-\n-#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class \" works\")\n+\t} \\\n+\ttest__run_end(!skip, TEST_LOCATION(), #class \" works\"); \\\n+} while (0)\n\n #define DIGIT \"0123456789\"\n #define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n@@ -32,37 +32,21 @@ static void test_ctype_##func(void) { \\\n \t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n \t\"\\x7f\"\n\n-TEST_CTYPE_FUNC(isdigit, DIGIT)\n-TEST_CTYPE_FUNC(isspace, \" \\n\\r\\t\")\n-TEST_CTYPE_FUNC(isalpha, LOWER UPPER)\n-TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)\n-TEST_CTYPE_FUNC(is_glob_special, \"*?[\\\\\")\n-TEST_CTYPE_FUNC(is_regex_special, \"$()*+.?[\\\\^{|\")\n-TEST_CTYPE_FUNC(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\")\n-TEST_CTYPE_FUNC(isascii, ASCII)\n-TEST_CTYPE_FUNC(islower, LOWER)\n-TEST_CTYPE_FUNC(isupper, UPPER)\n-TEST_CTYPE_FUNC(iscntrl, CNTRL)\n-TEST_CTYPE_FUNC(ispunct, PUNCT)\n-TEST_CTYPE_FUNC(isxdigit, DIGIT \"abcdefABCDEF\")\n-TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT \" \")\n-\n int cmd_main(int argc, const char **argv) {\n-\t/* Run all character type tests */\n-\tTEST_CHAR_CLASS(isspace);\n-\tTEST_CHAR_CLASS(isdigit);\n-\tTEST_CHAR_CLASS(isalpha);\n-\tTEST_CHAR_CLASS(isalnum);\n-\tTEST_CHAR_CLASS(is_glob_special);\n-\tTEST_CHAR_CLASS(is_regex_special);\n-\tTEST_CHAR_CLASS(is_pathspec_magic);\n-\tTEST_CHAR_CLASS(isascii);\n-\tTEST_CHAR_CLASS(islower);\n-\tTEST_CHAR_CLASS(isupper);\n-\tTEST_CHAR_CLASS(iscntrl);\n-\tTEST_CHAR_CLASS(ispunct);\n-\tTEST_CHAR_CLASS(isxdigit);\n-\tTEST_CHAR_CLASS(isprint);\n+\tTEST_CHAR_CLASS(isspace, \" \\n\\r\\t\");\n+\tTEST_CHAR_CLASS(isdigit, DIGIT);\n+\tTEST_CHAR_CLASS(isalpha, LOWER UPPER);\n+\tTEST_CHAR_CLASS(isalnum, LOWER UPPER DIGIT);\n+\tTEST_CHAR_CLASS(is_glob_special, \"*?[\\\\\");\n+\tTEST_CHAR_CLASS(is_regex_special, \"$()*+.?[\\\\^{|\");\n+\tTEST_CHAR_CLASS(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\");\n+\tTEST_CHAR_CLASS(isascii, ASCII);\n+\tTEST_CHAR_CLASS(islower, LOWER);\n+\tTEST_CHAR_CLASS(isupper, UPPER);\n+\tTEST_CHAR_CLASS(iscntrl, CNTRL);\n+\tTEST_CHAR_CLASS(ispunct, PUNCT);\n+\tTEST_CHAR_CLASS(isxdigit, DIGIT \"abcdefABCDEF\");\n+\tTEST_CHAR_CLASS(isprint, LOWER UPPER DIGIT PUNCT \" \");\n\n \treturn test_done();\n }\n--\n2.44.0\n\n"},{"id":"489306","messageId":"20240225112722.89221-4-l.s.r@web.de","threadId":"60991","inReplyTo":"20240225112722.89221-1-l.s.r@web.de","subject":"[PATCH 3/3] t-ctype: do one test per class and char","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-25T11:27:20Z","receivedAt":"2024-02-25T11:27:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Simplify TEST_CHAR_CLASS by using TEST for each character separately.\nThis increases the number of tests to 3598, but avoids the need for\nusing internal functions and test_msg() for custom messages.  The\nresulting macro has minimal test setup overhead.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/unit-tests/t-ctype.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\nindex 56dfefb68e..aa728175a6 100644\n--- a/t/unit-tests/t-ctype.c\n+++ b/t/unit-tests/t-ctype.c\n@@ -1,17 +1,12 @@\n #include \"test-lib.h\"\n\n #define TEST_CHAR_CLASS(class, string) do { \\\n-\tint skip = test__run_begin(); \\\n-\tif (!skip) { \\\n-\t\tfor (int i = 0; i < 256; i++) { \\\n-\t\t\tint expect = !!memchr(string, i, sizeof(string) - 1); \\\n-\t\t\tif (!check_int(class(i), ==, expect)) \\\n-\t\t\t\ttest_msg(\"       i: 0x%02x\", i); \\\n-\t\t} \\\n-\t\tif (!check(!class(EOF))) \\\n-\t\t\ttest_msg(\"      i: 0x%02x (EOF)\", EOF); \\\n+\tfor (int i = 0; i < 256; i++) { \\\n+\t\tint expect = !!memchr(string, i, sizeof(string) - 1); \\\n+\t\tTEST(check_int(class(i), ==, expect), \\\n+\t\t     #class \"(0x%02x) works\", i); \\\n \t} \\\n-\ttest__run_end(!skip, TEST_LOCATION(), #class \" works\"); \\\n+\tTEST(check(!class(EOF)), #class \"(EOF) works\"); \\\n } while (0)\n\n #define DIGIT \"0123456789\"\n--\n2.44.0\n\n"},{"id":"489319","messageId":"CAPig+cTvs7Ckg=k1T+N3N=yT840xM2DYf3LV9NZhii8+c1nkbw@mail.gmail.com","threadId":"60991","inReplyTo":"20240225112722.89221-2-l.s.r@web.de","subject":"Re: [PATCH 1/3] t-ctype: allow NUL anywhere in the specification string","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-25T18:05:32Z","receivedAt":"2024-02-25T18:05:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 25, 2024 at 6:27 AM René Scharfe <l.s.r@web.de> wrote:\n> Replace the custom function is_in() for looking up a character in the\n> specification string with memchr(3) and sizeof.  This is shorter,\n> simpler and allows NUL anywhere in the string, which may come in handy\n> if we ever want to support more character classes that contain it.\n>\n> Getting the string size using sizeof only works in a macro and with a\n> string constant, but that's exactly what we have and I don't see it\n> changing anytime soon.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\n> @@ -1,23 +1,11 @@\n>  /* Macro to test a character type */\n>  #define TEST_CTYPE_FUNC(func, string) \\\n\nTaking into consideration the commit message warning about string\nconstants, would it make sense to update the comment to mention that\nlimitation?\n\n    /* Test a character type. (Only use with string constants.) */\n    #define TEST_CTYPE_FUNC(func, string) \\\n\n>  static void test_ctype_##func(void) { \\\n>         for (int i = 0; i < 256; i++) { \\\n> -               if (!check_int(func(i), ==, is_in(string, i))) \\\n> +               int expect = !!memchr(string, i, sizeof(string) - 1); \\\n> +               if (!check_int(func(i), ==, expect)) \\\n>                         test_msg(\"       i: 0x%02x\", i); \\\n>         } \\\n>         if (!check(!func(EOF))) \\\n"},{"id":"489321","messageId":"5dea0535-4b21-483a-a10a-bfa8e9e85650@web.de","threadId":"60991","inReplyTo":"CAPig+cTvs7Ckg=k1T+N3N=yT840xM2DYf3LV9NZhii8+c1nkbw@mail.gmail.com","subject":"Re: [PATCH 1/3] t-ctype: allow NUL anywhere in the specification string","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-25T18:28:26Z","receivedAt":"2024-02-25T18:28:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.02.24 um 19:05 schrieb Eric Sunshine:\n> On Sun, Feb 25, 2024 at 6:27 AM René Scharfe <l.s.r@web.de> wrote:\n>> Replace the custom function is_in() for looking up a character in the\n>> specification string with memchr(3) and sizeof.  This is shorter,\n>> simpler and allows NUL anywhere in the string, which may come in handy\n>> if we ever want to support more character classes that contain it.\n>>\n>> Getting the string size using sizeof only works in a macro and with a\n>> string constant, but that's exactly what we have and I don't see it\n>> changing anytime soon.\n>>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>> diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\n>> @@ -1,23 +1,11 @@\n>>  /* Macro to test a character type */\n>>  #define TEST_CTYPE_FUNC(func, string) \\\n>\n> Taking into consideration the commit message warning about string\n> constants, would it make sense to update the comment to mention that\n> limitation?\n\nI think the temptation to pass a string pointer is low -- if only\nbecause there aren't any in this file.  But adding such a warning\ncan't hurt, so yeah, why not?\n\n>\n>     /* Test a character type. (Only use with string constants.) */\n>     #define TEST_CTYPE_FUNC(func, string) \\\n>\n>>  static void test_ctype_##func(void) { \\\n>>         for (int i = 0; i < 256; i++) { \\\n>> -               if (!check_int(func(i), ==, is_in(string, i))) \\\n>> +               int expect = !!memchr(string, i, sizeof(string) - 1); \\\n>> +               if (!check_int(func(i), ==, expect)) \\\n>>                         test_msg(\"       i: 0x%02x\", i); \\\n>>         } \\\n>>         if (!check(!func(EOF))) \\\n"},{"id":"489324","messageId":"CAPig+cThAYjf4DU40dY9jCnjL0FsVpYFhcsj9YdvW2Q=c=ZDBg@mail.gmail.com","threadId":"60991","inReplyTo":"5dea0535-4b21-483a-a10a-bfa8e9e85650@web.de","subject":"Re: [PATCH 1/3] t-ctype: allow NUL anywhere in the specification string","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-25T18:41:30Z","receivedAt":"2024-02-25T18:41:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 25, 2024 at 1:28 PM René Scharfe <l.s.r@web.de> wrote:\n> Am 25.02.24 um 19:05 schrieb Eric Sunshine:\n> > On Sun, Feb 25, 2024 at 6:27 AM René Scharfe <l.s.r@web.de> wrote:\n> >> Getting the string size using sizeof only works in a macro and with a\n> >> string constant, but that's exactly what we have and I don't see it\n> >> changing anytime soon.\n> >>\n> >>  /* Macro to test a character type */\n> >>  #define TEST_CTYPE_FUNC(func, string) \\\n> >\n> > Taking into consideration the commit message warning about string\n> > constants, would it make sense to update the comment to mention that\n> > limitation?\n> >\n> >     /* Test a character type. (Only use with string constants.) */\n> >     #define TEST_CTYPE_FUNC(func, string) \\\n> >>  static void test_ctype_##func(void) { \\\n> >>         for (int i = 0; i < 256; i++) { \\\n> >> +               int expect = !!memchr(string, i, sizeof(string) - 1); \\\n>\n> I think the temptation to pass a string pointer is low -- if only\n> because there aren't any in this file.  But adding such a warning\n> can't hurt, so yeah, why not?\n\nThe patch just posted[1] by SZEDER reminded me that, on this project,\nwe assume that the compiler is smart enough to replace\n`strlen(\"string-literal\")` with the constant `15`, so rather than\nworrying about updating comment to mention the sizeof() limitation,\nyou could perhaps just use `strlen(string)` instead of\n`sizeof(string)-1`?\n\n[1]: https://lore.kernel.org/git/20240225183452.1939334-1-szeder.dev@gmail.com/\n"},{"id":"489329","messageId":"20240225210038.GA3368483@coredump.intra.peff.net","threadId":"60991","inReplyTo":"CAPig+cThAYjf4DU40dY9jCnjL0FsVpYFhcsj9YdvW2Q=c=ZDBg@mail.gmail.com","subject":"Re: [PATCH 1/3] t-ctype: allow NUL anywhere in the specification string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-25T21:00:38Z","receivedAt":"2024-02-25T21:00:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 25, 2024 at 01:41:30PM -0500, Eric Sunshine wrote:\n\n> On Sun, Feb 25, 2024 at 1:28 PM René Scharfe <l.s.r@web.de> wrote:\n> > Am 25.02.24 um 19:05 schrieb Eric Sunshine:\n> > > On Sun, Feb 25, 2024 at 6:27 AM René Scharfe <l.s.r@web.de> wrote:\n> > >> Getting the string size using sizeof only works in a macro and with a\n> > >> string constant, but that's exactly what we have and I don't see it\n> > >> changing anytime soon.\n> > >>\n> > >>  /* Macro to test a character type */\n> > >>  #define TEST_CTYPE_FUNC(func, string) \\\n> > >\n> > > Taking into consideration the commit message warning about string\n> > > constants, would it make sense to update the comment to mention that\n> > > limitation?\n> > >\n> > >     /* Test a character type. (Only use with string constants.) */\n> > >     #define TEST_CTYPE_FUNC(func, string) \\\n> > >>  static void test_ctype_##func(void) { \\\n> > >>         for (int i = 0; i < 256; i++) { \\\n> > >> +               int expect = !!memchr(string, i, sizeof(string) - 1); \\\n> >\n> > I think the temptation to pass a string pointer is low -- if only\n> > because there aren't any in this file.  But adding such a warning\n> > can't hurt, so yeah, why not?\n> \n> The patch just posted[1] by SZEDER reminded me that, on this project,\n> we assume that the compiler is smart enough to replace\n> `strlen(\"string-literal\")` with the constant `15`, so rather than\n> worrying about updating comment to mention the sizeof() limitation,\n> you could perhaps just use `strlen(string)` instead of\n> `sizeof(string)-1`?\n\nThat would defeat the advertised purpose that we can handle embedded\nNULs, though. Whereas with sizeof(), I think a literal like \"foo\\0bar\"\nwould still have length 8.\n\n-Peff\n"},{"id":"489330","messageId":"CAPig+cRtXHJL76ZUg8bqwpVa48-rOOxB8wNrSDJwJkQyfUYDTg@mail.gmail.com","threadId":"60991","inReplyTo":"20240225210038.GA3368483@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] t-ctype: allow NUL anywhere in the specification string","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-25T21:02:35Z","receivedAt":"2024-02-25T21:02:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 25, 2024 at 4:00 PM Jeff King <peff@peff.net> wrote:\n> On Sun, Feb 25, 2024 at 01:41:30PM -0500, Eric Sunshine wrote:\n> > On Sun, Feb 25, 2024 at 1:28 PM René Scharfe <l.s.r@web.de> wrote:\n> > > Am 25.02.24 um 19:05 schrieb Eric Sunshine:\n> > > > Taking into consideration the commit message warning about string\n> > > > constants, would it make sense to update the comment to mention that\n> > > > limitation?\n> > >\n> > > I think the temptation to pass a string pointer is low -- if only\n> > > because there aren't any in this file.  But adding such a warning\n> > > can't hurt, so yeah, why not?\n> >\n> > The patch just posted[1] by SZEDER reminded me that, on this project,\n> > we assume that the compiler is smart enough to replace\n> > `strlen(\"string-literal\")` with the constant `15`, so rather than\n> > worrying about updating comment to mention the sizeof() limitation,\n> > you could perhaps just use `strlen(string)` instead of\n> > `sizeof(string)-1`?\n>\n> That would defeat the advertised purpose that we can handle embedded\n> NULs, though. Whereas with sizeof(), I think a literal like \"foo\\0bar\"\n> would still have length 8.\n\nTrue. Sorry for the noise.\n"},{"id":"489358","messageId":"CAP8UFD0Wi3ot-t0Q7ruMauwj4zkMfd89Xr9SmxYa4eQ3=2VKOw@mail.gmail.com","threadId":"60991","inReplyTo":"20240225112722.89221-4-l.s.r@web.de","subject":"Re: [PATCH 3/3] t-ctype: do one test per class and char","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-02-26T09:28:14Z","receivedAt":"2024-02-26T09:28:27Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Feb 25, 2024 at 12:27 PM René Scharfe <l.s.r@web.de> wrote:\n>\n> Simplify TEST_CHAR_CLASS by using TEST for each character separately.\n> This increases the number of tests to 3598,\n\nDoes this mean that when all the tests pass there will be 3598 lines\nof output on the terminal instead of 14 before this patch?\n\nIf that's the case, I don't like this.\n\n> but avoids the need for\n> using internal functions and test_msg() for custom messages.  The\n> resulting macro has minimal test setup overhead.\n\nYeah, the code looks definitely cleaner, but a clean output is important too.\n\nThanks!\n"},{"id":"489393","messageId":"d96aaf45-f073-42d0-b69c-703393634848@web.de","threadId":"60991","inReplyTo":"CAP8UFD0Wi3ot-t0Q7ruMauwj4zkMfd89Xr9SmxYa4eQ3=2VKOw@mail.gmail.com","subject":"Re: [PATCH 3/3] t-ctype: do one test per class and char","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-26T17:26:48Z","receivedAt":"2024-02-26T17:26:51Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 26.02.24 um 10:28 schrieb Christian Couder:\n> On Sun, Feb 25, 2024 at 12:27 PM René Scharfe <l.s.r@web.de> wrote:\n>>\n>> Simplify TEST_CHAR_CLASS by using TEST for each character separately.\n>> This increases the number of tests to 3598,\n>\n> Does this mean that when all the tests pass there will be 3598 lines\n> of output on the terminal instead of 14 before this patch?\n\nYes.\n\n> If that's the case, I don't like this.\n>\n>> but avoids the need for\n>> using internal functions and test_msg() for custom messages.  The\n>> resulting macro has minimal test setup overhead.\n>\n> Yeah, the code looks definitely cleaner, but a clean output is important too.\n\nThe output is clean as well, but there's a lot of it.  Perhaps too much.\nThe success messages are boring, though, and if all checks pass then the\nonly useful information is the status code.  A TAP harness like prove\nsummarizes that nicely:\n\n   $ prove t/unit-tests/bin/t-ctype\n   t/unit-tests/bin/t-ctype .. ok\n   All tests successful.\n   Files=1, Tests=3598,  0 wallclock secs ( 0.08 usr +  0.00 sys =  0.08 CPU)\n   Result: PASS\n\nFiltering out passing checks e.g. with \"| grep -v ^ok\" would help when\ndebugging a test failure. I vaguely miss the --immediate switch from the\nregular test library, however.\n\nRené\n"},{"id":"489396","messageId":"xmqq4jdvp1bg.fsf@gitster.g","threadId":"60991","inReplyTo":"d96aaf45-f073-42d0-b69c-703393634848@web.de","subject":"Re: [PATCH 3/3] t-ctype: do one test per class and char","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-26T17:44:51Z","receivedAt":"2024-02-26T17:44:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Filtering out passing checks e.g. with \"| grep -v ^ok\" would help when\n> debugging a test failure. I vaguely miss the --immediate switch from the\n> regular test library, however.\n\nYeah, \"-i\" is one of the most useful switches during debugging\ntests.\n"},{"id":"489408","messageId":"ZdzfYPim2SP22eeS@google.com","threadId":"60991","inReplyTo":"d96aaf45-f073-42d0-b69c-703393634848@web.de","subject":"Re: [PATCH 3/3] t-ctype: do one test per class and char","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-02-26T18:58:40Z","receivedAt":"2024-02-26T18:58:45Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.02.26 18:26, René Scharfe wrote:\n> Am 26.02.24 um 10:28 schrieb Christian Couder:\n> > On Sun, Feb 25, 2024 at 12:27 PM René Scharfe <l.s.r@web.de> wrote:\n> >>\n> >> Simplify TEST_CHAR_CLASS by using TEST for each character separately.\n> >> This increases the number of tests to 3598,\n> >\n> > Does this mean that when all the tests pass there will be 3598 lines\n> > of output on the terminal instead of 14 before this patch?\n> \n> Yes.\n> \n> > If that's the case, I don't like this.\n> >\n> >> but avoids the need for\n> >> using internal functions and test_msg() for custom messages.  The\n> >> resulting macro has minimal test setup overhead.\n> >\n> > Yeah, the code looks definitely cleaner, but a clean output is important too.\n> \n> The output is clean as well, but there's a lot of it.  Perhaps too much.\n> The success messages are boring, though, and if all checks pass then the\n> only useful information is the status code.  A TAP harness like prove\n> summarizes that nicely:\n> \n>    $ prove t/unit-tests/bin/t-ctype\n>    t/unit-tests/bin/t-ctype .. ok\n>    All tests successful.\n>    Files=1, Tests=3598,  0 wallclock secs ( 0.08 usr +  0.00 sys =  0.08 CPU)\n>    Result: PASS\n> \n> Filtering out passing checks e.g. with \"| grep -v ^ok\" would help when\n> debugging a test failure. I vaguely miss the --immediate switch from the\n> regular test library, however.\n\nYeah, I agree here. It's a lot of output but it's almost always going to\nbe consumed by a test harness rather than a human, and it's easy to\nfilter out the noise if someone does need to do some manual debugging.\n"},{"id":"489480","messageId":"CAP8UFD2t1KRo01eenK_RVndyVx5Vp9F4FepTgnR+mwhTGTvXnw@mail.gmail.com","threadId":"60991","inReplyTo":"ZdzfYPim2SP22eeS@google.com","subject":"Re: [PATCH 3/3] t-ctype: do one test per class and char","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-02-27T10:04:00Z","receivedAt":"2024-02-27T10:04:14Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Feb 26, 2024 at 7:58 PM Josh Steadmon <steadmon@google.com> wrote:\n>\n> On 2024.02.26 18:26, René Scharfe wrote:\n\n> > The output is clean as well, but there's a lot of it.  Perhaps too much.\n> > The success messages are boring, though, and if all checks pass then the\n> > only useful information is the status code.  A TAP harness like prove\n> > summarizes that nicely:\n> >\n> >    $ prove t/unit-tests/bin/t-ctype\n> >    t/unit-tests/bin/t-ctype .. ok\n> >    All tests successful.\n> >    Files=1, Tests=3598,  0 wallclock secs ( 0.08 usr +  0.00 sys =  0.08 CPU)\n> >    Result: PASS\n> >\n> > Filtering out passing checks e.g. with \"| grep -v ^ok\" would help when\n> > debugging a test failure. I vaguely miss the --immediate switch from the\n> > regular test library, however.\n>\n> Yeah, I agree here. It's a lot of output but it's almost always going to\n> be consumed by a test harness rather than a human, and it's easy to\n> filter out the noise if someone does need to do some manual debugging.\n\nYeah, I know about TAP harnesses like prove, but the most\nstraightforward way to run the unit tests is still `make unit-tests`\nin the t/ directory. Also when you add or change some tests, it's a\ngood idea to run `make unit-tests` to see what the output is, so you\nstill have to see that output quite often when you work on tests and\ngoing through 3598 of mostly useless output instead of just 14 isn't\nnice.\n"},{"id":"489799","messageId":"bd48f19b-0600-4e64-835b-98d3a97bb7f2@web.de","threadId":"60991","inReplyTo":"CAP8UFD2t1KRo01eenK_RVndyVx5Vp9F4FepTgnR+mwhTGTvXnw@mail.gmail.com","subject":"Re: [PATCH 3/3] t-ctype: do one test per class and char","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-02T22:00:00Z","receivedAt":"2024-03-02T22:00:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.02.24 um 11:04 schrieb Christian Couder:\n> On Mon, Feb 26, 2024 at 7:58 PM Josh Steadmon <steadmon@google.com> wrote:\n>>\n>> On 2024.02.26 18:26, René Scharfe wrote:\n>\n>>> The output is clean as well, but there's a lot of it.  Perhaps too much.\n>>> The success messages are boring, though, and if all checks pass then the\n>>> only useful information is the status code.  A TAP harness like prove\n>>> summarizes that nicely:\n>>>\n>>>    $ prove t/unit-tests/bin/t-ctype\n>>>    t/unit-tests/bin/t-ctype .. ok\n>>>    All tests successful.\n>>>    Files=1, Tests=3598,  0 wallclock secs ( 0.08 usr +  0.00 sys =  0.08 CPU)\n>>>    Result: PASS\n>>>\n>>> Filtering out passing checks e.g. with \"| grep -v ^ok\" would help when\n>>> debugging a test failure. I vaguely miss the --immediate switch from the\n>>> regular test library, however.\n>>\n>> Yeah, I agree here. It's a lot of output but it's almost always going to\n>> be consumed by a test harness rather than a human, and it's easy to\n>> filter out the noise if someone does need to do some manual debugging.\n>\n> Yeah, I know about TAP harnesses like prove, but the most\n> straightforward way to run the unit tests is still `make unit-tests`\n> in the t/ directory. Also when you add or change some tests, it's a\n> good idea to run `make unit-tests` to see what the output is, so you\n> still have to see that output quite often when you work on tests and\n> going through 3598 of mostly useless output instead of just 14 isn't\n> nice.\n\nI was starting the programs from t/unit-tests/bin/ individually because\nI didn't know 'make unit-tests' exists.  This is much nicer, thank you!\nEspecially after adding 'DEFAULT_UNIT_TEST_TARGET = unit-tests-prove' to\nconfig.mak to complement the 'DEFAULT_TEST_TARGET = prove' I added long\nago.  It would be even nicer if the former was the default when the\nlatter is set.\n\nAs unit tests are added, their output is surely going to grow to\nmultiple screens with or without prove, no?  So someone writing or\ndebugging tests will still go back to starting then individually\neventually.\n\nThe size of the output in itself is not a problem, I assume, but that\nmost of it is useless -- details of successful tests are uninteresting.\nA test harness can aggregate the output, but prove annoyed me when used\nwith the regular tests by also aggregating error output and only showing\nthe numbers of failed tests.  Finding their names involved running the\ntest script again without prove.  Turns out it has an option for that.\nAdded 'GIT_PROVE_OPTS = --failures' to config.mak as well, will see if\nit helps.\n\nIs it too much to ask developers to use a test harness?  Perhaps: It's\nyet another dependency and not enabled by default.\n\nWhat's the right level of aggregation and how do we achieve it?\nGrouping by class is natural and follows the test definition.  We\ncould stop after patch 2.  Dunno.\n\nRené\n"},{"id":"489806","messageId":"20240303101330.20187-3-l.s.r@web.de","threadId":"60991","inReplyTo":"20240303101330.20187-1-l.s.r@web.de","subject":"[PATCH v2 2/4] t-ctype: simplify EOF check","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-03T10:13:26Z","receivedAt":"2024-03-03T10:13:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"EOF is not a member of any character class.  If a classifier function\nreturns a non-zero result for it, presumably by mistake, then the unit\ntest check reports:\n\n   # check \"!iseof(EOF)\" failed at t/unit-tests/t-ctype.c:53\n   #       i: 0xffffffff (EOF)\n\nThe numeric value of EOF is not particularly interesting in this\ncontext.  Stop printing the second line.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/unit-tests/t-ctype.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\nindex 35473c41d8..f0d61d6eb2 100644\n--- a/t/unit-tests/t-ctype.c\n+++ b/t/unit-tests/t-ctype.c\n@@ -10,8 +10,7 @@ static void test_ctype_##func(void) { \\\n \t\tif (!check_int(func(i), ==, !!memchr(string, i, len))) \\\n \t\t\ttest_msg(\"       i: 0x%02x\", i); \\\n \t} \\\n-\tif (!check(!func(EOF))) \\\n-\t\t\ttest_msg(\"      i: 0x%02x (EOF)\", EOF); \\\n+\tcheck(!func(EOF)); \\\n }\n\n #define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class \" works\")\n--\n2.44.0\n\n"},{"id":"489807","messageId":"20240303101330.20187-4-l.s.r@web.de","threadId":"60991","inReplyTo":"20240303101330.20187-1-l.s.r@web.de","subject":"[PATCH v2 3/4] t-ctype: align output of i","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-03T10:13:27Z","receivedAt":"2024-03-03T10:13:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The unit test reports misclassified characters like this:\n\n   # check \"isdigit(i) == !!memchr(\"123456789\", i, len)\" failed at t/unit-tests/t-ctype.c:36\n   #    left: 1\n   #   right: 0\n   #        i: 0x30\n\nReduce the indent of i to put its colon directly below the ones in the\npreceding lines for consistency.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/unit-tests/t-ctype.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\nindex f0d61d6eb2..02d8569aa3 100644\n--- a/t/unit-tests/t-ctype.c\n+++ b/t/unit-tests/t-ctype.c\n@@ -8,7 +8,7 @@ static void test_ctype_##func(void) { \\\n \t\tBUILD_ASSERT_OR_ZERO(sizeof(string[0]) == sizeof(char)); \\\n \tfor (int i = 0; i < 256; i++) { \\\n \t\tif (!check_int(func(i), ==, !!memchr(string, i, len))) \\\n-\t\t\ttest_msg(\"       i: 0x%02x\", i); \\\n+\t\t\ttest_msg(\"      i: 0x%02x\", i); \\\n \t} \\\n \tcheck(!func(EOF)); \\\n }\n--\n2.44.0\n\n"},{"id":"489805","messageId":"20240303101330.20187-5-l.s.r@web.de","threadId":"60991","inReplyTo":"20240303101330.20187-1-l.s.r@web.de","subject":"[PATCH v2 4/4] t-ctype: avoid duplicating class names","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-03T10:13:28Z","receivedAt":"2024-03-03T10:13:53Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"TEST_CTYPE_FUNC defines a function for testing a character classifier,\nTEST_CHAR_CLASS calls it, causing the class name to be mentioned twice.\n\nAvoid the need to define a class-specific function by letting\nTEST_CHAR_CLASS do all the work.  This is done by using the internal\nfunctions test__run_begin() and test__run_end(), but they do exist to be\nused in test macros after all.\n\nAlternatively we could unroll the loop to provide a very long expression\nthat tests all 256 characters and EOF and hand that to TEST, but that\nseems awkward and hard to read.\n\nNo change of behavior or output intended.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/unit-tests/t-ctype.c | 64 ++++++++++++++++--------------------------\n 1 file changed, 24 insertions(+), 40 deletions(-)\n\ndiff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\nindex 02d8569aa3..d6ac1fe678 100644\n--- a/t/unit-tests/t-ctype.c\n+++ b/t/unit-tests/t-ctype.c\n@@ -1,19 +1,19 @@\n #include \"test-lib.h\"\n\n-/* Macro to test a character type */\n-#define TEST_CTYPE_FUNC(func, string) \\\n-static void test_ctype_##func(void) { \\\n+#define TEST_CHAR_CLASS(class, string) do { \\\n \tsize_t len = ARRAY_SIZE(string) - 1 + \\\n \t\tBUILD_ASSERT_OR_ZERO(ARRAY_SIZE(string) > 0) + \\\n \t\tBUILD_ASSERT_OR_ZERO(sizeof(string[0]) == sizeof(char)); \\\n-\tfor (int i = 0; i < 256; i++) { \\\n-\t\tif (!check_int(func(i), ==, !!memchr(string, i, len))) \\\n-\t\t\ttest_msg(\"      i: 0x%02x\", i); \\\n+\tint skip = test__run_begin(); \\\n+\tif (!skip) { \\\n+\t\tfor (int i = 0; i < 256; i++) { \\\n+\t\t\tif (!check_int(class(i), ==, !!memchr(string, i, len)))\\\n+\t\t\t\ttest_msg(\"      i: 0x%02x\", i); \\\n+\t\t} \\\n+\t\tcheck(!class(EOF)); \\\n \t} \\\n-\tcheck(!func(EOF)); \\\n-}\n-\n-#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class \" works\")\n+\ttest__run_end(!skip, TEST_LOCATION(), #class \" works\"); \\\n+} while (0)\n\n #define DIGIT \"0123456789\"\n #define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n@@ -33,37 +33,21 @@ static void test_ctype_##func(void) { \\\n \t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n \t\"\\x7f\"\n\n-TEST_CTYPE_FUNC(isdigit, DIGIT)\n-TEST_CTYPE_FUNC(isspace, \" \\n\\r\\t\")\n-TEST_CTYPE_FUNC(isalpha, LOWER UPPER)\n-TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)\n-TEST_CTYPE_FUNC(is_glob_special, \"*?[\\\\\")\n-TEST_CTYPE_FUNC(is_regex_special, \"$()*+.?[\\\\^{|\")\n-TEST_CTYPE_FUNC(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\")\n-TEST_CTYPE_FUNC(isascii, ASCII)\n-TEST_CTYPE_FUNC(islower, LOWER)\n-TEST_CTYPE_FUNC(isupper, UPPER)\n-TEST_CTYPE_FUNC(iscntrl, CNTRL)\n-TEST_CTYPE_FUNC(ispunct, PUNCT)\n-TEST_CTYPE_FUNC(isxdigit, DIGIT \"abcdefABCDEF\")\n-TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT \" \")\n-\n int cmd_main(int argc, const char **argv) {\n-\t/* Run all character type tests */\n-\tTEST_CHAR_CLASS(isspace);\n-\tTEST_CHAR_CLASS(isdigit);\n-\tTEST_CHAR_CLASS(isalpha);\n-\tTEST_CHAR_CLASS(isalnum);\n-\tTEST_CHAR_CLASS(is_glob_special);\n-\tTEST_CHAR_CLASS(is_regex_special);\n-\tTEST_CHAR_CLASS(is_pathspec_magic);\n-\tTEST_CHAR_CLASS(isascii);\n-\tTEST_CHAR_CLASS(islower);\n-\tTEST_CHAR_CLASS(isupper);\n-\tTEST_CHAR_CLASS(iscntrl);\n-\tTEST_CHAR_CLASS(ispunct);\n-\tTEST_CHAR_CLASS(isxdigit);\n-\tTEST_CHAR_CLASS(isprint);\n+\tTEST_CHAR_CLASS(isspace, \" \\n\\r\\t\");\n+\tTEST_CHAR_CLASS(isdigit, DIGIT);\n+\tTEST_CHAR_CLASS(isalpha, LOWER UPPER);\n+\tTEST_CHAR_CLASS(isalnum, LOWER UPPER DIGIT);\n+\tTEST_CHAR_CLASS(is_glob_special, \"*?[\\\\\");\n+\tTEST_CHAR_CLASS(is_regex_special, \"$()*+.?[\\\\^{|\");\n+\tTEST_CHAR_CLASS(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\");\n+\tTEST_CHAR_CLASS(isascii, ASCII);\n+\tTEST_CHAR_CLASS(islower, LOWER);\n+\tTEST_CHAR_CLASS(isupper, UPPER);\n+\tTEST_CHAR_CLASS(iscntrl, CNTRL);\n+\tTEST_CHAR_CLASS(ispunct, PUNCT);\n+\tTEST_CHAR_CLASS(isxdigit, DIGIT \"abcdefABCDEF\");\n+\tTEST_CHAR_CLASS(isprint, LOWER UPPER DIGIT PUNCT \" \");\n\n \treturn test_done();\n }\n--\n2.44.0\n\n"},{"id":"489808","messageId":"20240303101330.20187-1-l.s.r@web.de","threadId":"60991","inReplyTo":"20240225112722.89221-1-l.s.r@web.de","subject":"[PATCH v2 0/4] t-ctype: simplify unit test definitions","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-03T10:13:24Z","receivedAt":"2024-03-03T10:13:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Simplify the ctype unit tests to allow combining specification strings\nin any order and no longer require repeating class names.\n\nChanges since v1:\n* Added checks to guard string length calculation using sizeof.\n* Kept the definition string in the error output.\n* Added patches 2 and 3 with further cosmetic changes.\n* Dropped the last patch with the huge output for now, as it needs\n  further thought.\n\n  t-ctype: allow NUL anywhere in the specification string\n  t-ctype: simplify EOF check\n  t-ctype: align output of i\n  t-ctype: avoid duplicating class names\n\n t/unit-tests/t-ctype.c | 81 ++++++++++++++----------------------------\n 1 file changed, 27 insertions(+), 54 deletions(-)\n\nRange-Diff gegen v1:\n1:  ae9b5468db ! 1:  28fba8bda9 t-ctype: allow NUL anywhere in the specification string\n    @@ Commit message\n         if we ever want to support more character classes that contain it.\n\n         Getting the string size using sizeof only works in a macro and with a\n    -    string constant, but that's exactly what we have and I don't see it\n    -    changing anytime soon.\n    +    string constant.  Use ARRAY_SIZE and compile-time checks to make sure we\n    +    are not passed a string pointer.\n\n      ## t/unit-tests/t-ctype.c ##\n     @@\n    @@ t/unit-tests/t-ctype.c\n      /* Macro to test a character type */\n      #define TEST_CTYPE_FUNC(func, string) \\\n      static void test_ctype_##func(void) { \\\n    ++\tsize_t len = ARRAY_SIZE(string) - 1 + \\\n    ++\t\tBUILD_ASSERT_OR_ZERO(ARRAY_SIZE(string) > 0) + \\\n    ++\t\tBUILD_ASSERT_OR_ZERO(sizeof(string[0]) == sizeof(char)); \\\n      \tfor (int i = 0; i < 256; i++) { \\\n     -\t\tif (!check_int(func(i), ==, is_in(string, i))) \\\n    -+\t\tint expect = !!memchr(string, i, sizeof(string) - 1); \\\n    -+\t\tif (!check_int(func(i), ==, expect)) \\\n    ++\t\tif (!check_int(func(i), ==, !!memchr(string, i, len))) \\\n      \t\t\ttest_msg(\"       i: 0x%02x\", i); \\\n      \t} \\\n      \tif (!check(!func(EOF))) \\\n-:  ---------- > 2:  1eab0d289c t-ctype: simplify EOF check\n-:  ---------- > 3:  901312e980 t-ctype: align output of i\n2:  688e6b6cb5 ! 4:  5dac51cfeb t-ctype: avoid duplicating class names\n    @@ t/unit-tests/t-ctype.c\n     -/* Macro to test a character type */\n     -#define TEST_CTYPE_FUNC(func, string) \\\n     -static void test_ctype_##func(void) { \\\n    --\tfor (int i = 0; i < 256; i++) { \\\n    --\t\tint expect = !!memchr(string, i, sizeof(string) - 1); \\\n    --\t\tif (!check_int(func(i), ==, expect)) \\\n    --\t\t\ttest_msg(\"       i: 0x%02x\", i); \\\n    --\t} \\\n    --\tif (!check(!func(EOF))) \\\n     +#define TEST_CHAR_CLASS(class, string) do { \\\n    + \tsize_t len = ARRAY_SIZE(string) - 1 + \\\n    + \t\tBUILD_ASSERT_OR_ZERO(ARRAY_SIZE(string) > 0) + \\\n    + \t\tBUILD_ASSERT_OR_ZERO(sizeof(string[0]) == sizeof(char)); \\\n    +-\tfor (int i = 0; i < 256; i++) { \\\n    +-\t\tif (!check_int(func(i), ==, !!memchr(string, i, len))) \\\n    +-\t\t\ttest_msg(\"      i: 0x%02x\", i); \\\n     +\tint skip = test__run_begin(); \\\n     +\tif (!skip) { \\\n     +\t\tfor (int i = 0; i < 256; i++) { \\\n    -+\t\t\tint expect = !!memchr(string, i, sizeof(string) - 1); \\\n    -+\t\t\tif (!check_int(class(i), ==, expect)) \\\n    -+\t\t\t\ttest_msg(\"       i: 0x%02x\", i); \\\n    ++\t\t\tif (!check_int(class(i), ==, !!memchr(string, i, len)))\\\n    ++\t\t\t\ttest_msg(\"      i: 0x%02x\", i); \\\n     +\t\t} \\\n    -+\t\tif (!check(!class(EOF))) \\\n    - \t\t\ttest_msg(\"      i: 0x%02x (EOF)\", EOF); \\\n    ++\t\tcheck(!class(EOF)); \\\n    + \t} \\\n    +-\tcheck(!func(EOF)); \\\n     -}\n     -\n     -#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class \" works\")\n    -+\t} \\\n     +\ttest__run_end(!skip, TEST_LOCATION(), #class \" works\"); \\\n     +} while (0)\n\n3:  caf2acbd09 < -:  ---------- t-ctype: do one test per class and char\n--\n2.44.0\n\n"},{"id":"489809","messageId":"20240303101330.20187-2-l.s.r@web.de","threadId":"60991","inReplyTo":"20240303101330.20187-1-l.s.r@web.de","subject":"[PATCH v2 1/4] t-ctype: allow NUL anywhere in the specification string","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-03T10:13:25Z","receivedAt":"2024-03-03T10:13:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Replace the custom function is_in() for looking up a character in the\nspecification string with memchr(3) and sizeof.  This is shorter,\nsimpler and allows NUL anywhere in the string, which may come in handy\nif we ever want to support more character classes that contain it.\n\nGetting the string size using sizeof only works in a macro and with a\nstring constant.  Use ARRAY_SIZE and compile-time checks to make sure we\nare not passed a string pointer.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/unit-tests/t-ctype.c | 18 ++++--------------\n 1 file changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\nindex f315489984..35473c41d8 100644\n--- a/t/unit-tests/t-ctype.c\n+++ b/t/unit-tests/t-ctype.c\n@@ -1,23 +1,13 @@\n #include \"test-lib.h\"\n\n-static int is_in(const char *s, int ch)\n-{\n-\t/*\n-\t * We can't find NUL using strchr. Accept it as the first\n-\t * character in the spec -- there are no empty classes.\n-\t */\n-\tif (ch == '\\0')\n-\t\treturn ch == *s;\n-\tif (*s == '\\0')\n-\t\ts++;\n-\treturn !!strchr(s, ch);\n-}\n-\n /* Macro to test a character type */\n #define TEST_CTYPE_FUNC(func, string) \\\n static void test_ctype_##func(void) { \\\n+\tsize_t len = ARRAY_SIZE(string) - 1 + \\\n+\t\tBUILD_ASSERT_OR_ZERO(ARRAY_SIZE(string) > 0) + \\\n+\t\tBUILD_ASSERT_OR_ZERO(sizeof(string[0]) == sizeof(char)); \\\n \tfor (int i = 0; i < 256; i++) { \\\n-\t\tif (!check_int(func(i), ==, is_in(string, i))) \\\n+\t\tif (!check_int(func(i), ==, !!memchr(string, i, len))) \\\n \t\t\ttest_msg(\"       i: 0x%02x\", i); \\\n \t} \\\n \tif (!check(!func(EOF))) \\\n--\n2.44.0\n\n"},{"id":"489852","messageId":"CAP8UFD395xDjgqxg=S7dZrbnsCNsMEqQbKBipMU4m45CfT8f0w@mail.gmail.com","threadId":"60991","inReplyTo":"20240303101330.20187-1-l.s.r@web.de","subject":"Re: [PATCH v2 0/4] t-ctype: simplify unit test definitions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-03-04T09:25:20Z","receivedAt":"2024-03-04T09:25:34Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Mar 3, 2024 at 11:13 AM René Scharfe <l.s.r@web.de> wrote:\n>\n> Simplify the ctype unit tests to allow combining specification strings\n> in any order and no longer require repeating class names.\n>\n> Changes since v1:\n> * Added checks to guard string length calculation using sizeof.\n> * Kept the definition string in the error output.\n> * Added patches 2 and 3 with further cosmetic changes.\n> * Dropped the last patch with the huge output for now, as it needs\n>   further thought.\n\nThis V2 looks good to me. Acked.\n"},{"id":"489856","messageId":"0947cb09-8b07-4fcd-bbe2-ae37c2cd5ec7@gmail.com","threadId":"60991","inReplyTo":"20240303101330.20187-5-l.s.r@web.de","subject":"Re: [PATCH v2 4/4] t-ctype: avoid duplicating class names","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-03-04T09:51:35Z","receivedAt":"2024-03-04T09:51:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi René\n\nOn 03/03/2024 10:13, René Scharfe wrote:\n> TEST_CTYPE_FUNC defines a function for testing a character classifier,\n> TEST_CHAR_CLASS calls it, causing the class name to be mentioned twice.\n> \n> Avoid the need to define a class-specific function by letting\n> TEST_CHAR_CLASS do all the work.  This is done by using the internal\n> functions test__run_begin() and test__run_end(), but they do exist to be\n> used in test macros after all.\n\nThose internal functions exist to implement the TEST() macro, they are \nnot really intended for use outside that (which is why they are marked \nas private in the header file). If we ever want to update the \nimplementation of TEST() it will be a lot harder if we're using the \ninternal implementation directly in test files. Unit tests should be \nwrapping TEST() if it is appropriate but not the internal implementation \ndirectly.\n\nIdeally we wouldn't need TEST_CTYPE_FUNC as there would only be a single \nfunction that was passed a ctype predicate, an input array and an array \nof expected results. Unfortunately I don't think that is possible due \nthe the way the ctype predicates are implemented. Having separate macros \nto define the test function and to run the test is annoying but I don't \nthink it is really worth exposing the internal implementation just to \navoid it.\n\nThe other patches here look like useful improvements - thanks.\n\nBest Wishes\n\nPhillip\n\n> Alternatively we could unroll the loop to provide a very long expression\n> that tests all 256 characters and EOF and hand that to TEST, but that\n> seems awkward and hard to read.\n> \n> No change of behavior or output intended.\n> \n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>   t/unit-tests/t-ctype.c | 64 ++++++++++++++++--------------------------\n>   1 file changed, 24 insertions(+), 40 deletions(-)\n> \n> diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\n> index 02d8569aa3..d6ac1fe678 100644\n> --- a/t/unit-tests/t-ctype.c\n> +++ b/t/unit-tests/t-ctype.c\n> @@ -1,19 +1,19 @@\n>   #include \"test-lib.h\"\n> \n> -/* Macro to test a character type */\n> -#define TEST_CTYPE_FUNC(func, string) \\\n> -static void test_ctype_##func(void) { \\\n> +#define TEST_CHAR_CLASS(class, string) do { \\\n>   \tsize_t len = ARRAY_SIZE(string) - 1 + \\\n>   \t\tBUILD_ASSERT_OR_ZERO(ARRAY_SIZE(string) > 0) + \\\n>   \t\tBUILD_ASSERT_OR_ZERO(sizeof(string[0]) == sizeof(char)); \\\n> -\tfor (int i = 0; i < 256; i++) { \\\n> -\t\tif (!check_int(func(i), ==, !!memchr(string, i, len))) \\\n> -\t\t\ttest_msg(\"      i: 0x%02x\", i); \\\n> +\tint skip = test__run_begin(); \\\n> +\tif (!skip) { \\\n> +\t\tfor (int i = 0; i < 256; i++) { \\\n> +\t\t\tif (!check_int(class(i), ==, !!memchr(string, i, len)))\\\n> +\t\t\t\ttest_msg(\"      i: 0x%02x\", i); \\\n> +\t\t} \\\n> +\t\tcheck(!class(EOF)); \\\n>   \t} \\\n> -\tcheck(!func(EOF)); \\\n> -}\n> -\n> -#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class \" works\")\n> +\ttest__run_end(!skip, TEST_LOCATION(), #class \" works\"); \\\n> +} while (0)\n> \n>   #define DIGIT \"0123456789\"\n>   #define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n> @@ -33,37 +33,21 @@ static void test_ctype_##func(void) { \\\n>   \t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n>   \t\"\\x7f\"\n> \n> -TEST_CTYPE_FUNC(isdigit, DIGIT)\n> -TEST_CTYPE_FUNC(isspace, \" \\n\\r\\t\")\n> -TEST_CTYPE_FUNC(isalpha, LOWER UPPER)\n> -TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)\n> -TEST_CTYPE_FUNC(is_glob_special, \"*?[\\\\\")\n> -TEST_CTYPE_FUNC(is_regex_special, \"$()*+.?[\\\\^{|\")\n> -TEST_CTYPE_FUNC(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\")\n> -TEST_CTYPE_FUNC(isascii, ASCII)\n> -TEST_CTYPE_FUNC(islower, LOWER)\n> -TEST_CTYPE_FUNC(isupper, UPPER)\n> -TEST_CTYPE_FUNC(iscntrl, CNTRL)\n> -TEST_CTYPE_FUNC(ispunct, PUNCT)\n> -TEST_CTYPE_FUNC(isxdigit, DIGIT \"abcdefABCDEF\")\n> -TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT \" \")\n> -\n>   int cmd_main(int argc, const char **argv) {\n> -\t/* Run all character type tests */\n> -\tTEST_CHAR_CLASS(isspace);\n> -\tTEST_CHAR_CLASS(isdigit);\n> -\tTEST_CHAR_CLASS(isalpha);\n> -\tTEST_CHAR_CLASS(isalnum);\n> -\tTEST_CHAR_CLASS(is_glob_special);\n> -\tTEST_CHAR_CLASS(is_regex_special);\n> -\tTEST_CHAR_CLASS(is_pathspec_magic);\n> -\tTEST_CHAR_CLASS(isascii);\n> -\tTEST_CHAR_CLASS(islower);\n> -\tTEST_CHAR_CLASS(isupper);\n> -\tTEST_CHAR_CLASS(iscntrl);\n> -\tTEST_CHAR_CLASS(ispunct);\n> -\tTEST_CHAR_CLASS(isxdigit);\n> -\tTEST_CHAR_CLASS(isprint);\n> +\tTEST_CHAR_CLASS(isspace, \" \\n\\r\\t\");\n> +\tTEST_CHAR_CLASS(isdigit, DIGIT);\n> +\tTEST_CHAR_CLASS(isalpha, LOWER UPPER);\n> +\tTEST_CHAR_CLASS(isalnum, LOWER UPPER DIGIT);\n> +\tTEST_CHAR_CLASS(is_glob_special, \"*?[\\\\\");\n> +\tTEST_CHAR_CLASS(is_regex_special, \"$()*+.?[\\\\^{|\");\n> +\tTEST_CHAR_CLASS(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\");\n> +\tTEST_CHAR_CLASS(isascii, ASCII);\n> +\tTEST_CHAR_CLASS(islower, LOWER);\n> +\tTEST_CHAR_CLASS(isupper, UPPER);\n> +\tTEST_CHAR_CLASS(iscntrl, CNTRL);\n> +\tTEST_CHAR_CLASS(ispunct, PUNCT);\n> +\tTEST_CHAR_CLASS(isxdigit, DIGIT \"abcdefABCDEF\");\n> +\tTEST_CHAR_CLASS(isprint, LOWER UPPER DIGIT PUNCT \" \");\n> \n>   \treturn test_done();\n>   }\n> --\n> 2.44.0\n> \n"},{"id":"489864","messageId":"CAP8UFD2M2+x-pRX2nzCXbLW=nKcW4_RWc9qua5q-fU8QbGu1oA@mail.gmail.com","threadId":"60991","inReplyTo":"bd48f19b-0600-4e64-835b-98d3a97bb7f2@web.de","subject":"Re: [PATCH 3/3] t-ctype: do one test per class and char","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-03-04T10:00:45Z","receivedAt":"2024-03-04T10:00:59Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Mar 2, 2024 at 11:00 PM René Scharfe <l.s.r@web.de> wrote:\n>\n> Am 27.02.24 um 11:04 schrieb Christian Couder:\n\n> > Yeah, I know about TAP harnesses like prove, but the most\n> > straightforward way to run the unit tests is still `make unit-tests`\n> > in the t/ directory. Also when you add or change some tests, it's a\n> > good idea to run `make unit-tests` to see what the output is, so you\n> > still have to see that output quite often when you work on tests and\n> > going through 3598 of mostly useless output instead of just 14 isn't\n> > nice.\n>\n> I was starting the programs from t/unit-tests/bin/ individually because\n> I didn't know 'make unit-tests' exists.  This is much nicer, thank you!\n> Especially after adding 'DEFAULT_UNIT_TEST_TARGET = unit-tests-prove' to\n> config.mak to complement the 'DEFAULT_TEST_TARGET = prove' I added long\n> ago.  It would be even nicer if the former was the default when the\n> latter is set.\n>\n> As unit tests are added, their output is surely going to grow to\n> multiple screens with or without prove, no?  So someone writing or\n> debugging tests will still go back to starting then individually\n> eventually.\n\nWhen t-ctype will be run individually from t/unit-tests/bin/, for\nexample when adding or debugging ctype tests, it would still be better\nif there are only 14 lines in its output rather than 3598.\n\n> The size of the output in itself is not a problem, I assume, but that\n> most of it is useless -- details of successful tests are uninteresting.\n> A test harness can aggregate the output, but prove annoyed me when used\n> with the regular tests by also aggregating error output and only showing\n> the numbers of failed tests.  Finding their names involved running the\n> test script again without prove.  Turns out it has an option for that.\n> Added 'GIT_PROVE_OPTS = --failures' to config.mak as well, will see if\n> it helps.\n>\n> Is it too much to ask developers to use a test harness?  Perhaps: It's\n> yet another dependency and not enabled by default.\n\nYeah, it's a dependency, and when running CI tests, it's sometimes\nbetter and simpler to have the canonical output rather than having the\noutput processed by a test harness.\n\nAlso if we add some verbose or immediate modes (like -v and -i with\nthe shell test framework) or perhaps other kinds of modes (checking\nfor memory leaks or other errors using existing tools for example),\nthese modes might not interact nicely with test harnesses but still be\nuseful. Requiring to always use a test harness might restrict us for\nno good reason.\n\nAnd anyway it doesn't make sense to have meaningful messages as second\narguments to the TEST() macros if we always want to use a test harness\nthat just discards them. Either:\n\n  - we decide that we will always use some test harness, and then we\nmight just want to remove that second argument and yeah we can have\nthousands of tests output lines from a single binary, or\n  - we acknowledge that we don't always want to use a test harness,\nand then we want a relatively short and meaningful output from a\nsingle binary.\n\n> What's the right level of aggregation and how do we achieve it?\n> Grouping by class is natural and follows the test definition.  We\n> could stop after patch 2.  Dunno.\n\nI am Ok with just removing this patch like you did in v2. Thanks.\n"},{"id":"489909","messageId":"ZeYUZZ6Z8VtYnBn7@google.com","threadId":"60991","inReplyTo":"bd48f19b-0600-4e64-835b-98d3a97bb7f2@web.de","subject":"Re: [PATCH 3/3] t-ctype: do one test per class and char","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-03-04T18:35:17Z","receivedAt":"2024-03-04T18:35:23Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.03.02 23:00, René Scharfe wrote:\n> Am 27.02.24 um 11:04 schrieb Christian Couder:\n> > On Mon, Feb 26, 2024 at 7:58 PM Josh Steadmon <steadmon@google.com> wrote:\n> >>\n> >> On 2024.02.26 18:26, René Scharfe wrote:\n> >\n> >>> The output is clean as well, but there's a lot of it.  Perhaps too much.\n> >>> The success messages are boring, though, and if all checks pass then the\n> >>> only useful information is the status code.  A TAP harness like prove\n> >>> summarizes that nicely:\n> >>>\n> >>>    $ prove t/unit-tests/bin/t-ctype\n> >>>    t/unit-tests/bin/t-ctype .. ok\n> >>>    All tests successful.\n> >>>    Files=1, Tests=3598,  0 wallclock secs ( 0.08 usr +  0.00 sys =  0.08 CPU)\n> >>>    Result: PASS\n> >>>\n> >>> Filtering out passing checks e.g. with \"| grep -v ^ok\" would help when\n> >>> debugging a test failure. I vaguely miss the --immediate switch from the\n> >>> regular test library, however.\n> >>\n> >> Yeah, I agree here. It's a lot of output but it's almost always going to\n> >> be consumed by a test harness rather than a human, and it's easy to\n> >> filter out the noise if someone does need to do some manual debugging.\n> >\n> > Yeah, I know about TAP harnesses like prove, but the most\n> > straightforward way to run the unit tests is still `make unit-tests`\n> > in the t/ directory. Also when you add or change some tests, it's a\n> > good idea to run `make unit-tests` to see what the output is, so you\n> > still have to see that output quite often when you work on tests and\n> > going through 3598 of mostly useless output instead of just 14 isn't\n> > nice.\n> \n> I was starting the programs from t/unit-tests/bin/ individually because\n> I didn't know 'make unit-tests' exists.  This is much nicer, thank you!\n> Especially after adding 'DEFAULT_UNIT_TEST_TARGET = unit-tests-prove' to\n> config.mak to complement the 'DEFAULT_TEST_TARGET = prove' I added long\n> ago.  It would be even nicer if the former was the default when the\n> latter is set.\n\nAfter js/unit-test-suite-runner [1] is merged, then using\n'DEFAULT_TEST_TARGET = prove' will also run the unit tests alongside the\nshell test suite.\n\n[1] https://lore.kernel.org/git/cover.1708728717.git.steadmon@google.com/\n"},{"id":"489913","messageId":"xmqq5xy1q1gp.fsf@gitster.g","threadId":"60991","inReplyTo":"ZeYUZZ6Z8VtYnBn7@google.com","subject":"Re: [PATCH 3/3] t-ctype: do one test per class and char","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-04T18:46:46Z","receivedAt":"2024-03-04T18:46:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> On 2024.03.02 23:00, René Scharfe wrote:\n> ...\n>> I was starting the programs from t/unit-tests/bin/ individually because\n>> I didn't know 'make unit-tests' exists.  This is much nicer, thank you!\n>> Especially after adding 'DEFAULT_UNIT_TEST_TARGET = unit-tests-prove' to\n>> config.mak to complement the 'DEFAULT_TEST_TARGET = prove' I added long\n>> ago.  It would be even nicer if the former was the default when the\n>> latter is set.\n>\n> After js/unit-test-suite-runner [1] is merged, then using\n> 'DEFAULT_TEST_TARGET = prove' will also run the unit tests alongside the\n> shell test suite.\n>\n> [1] https://lore.kernel.org/git/cover.1708728717.git.steadmon@google.com/\n\nNice ;-).\n"},{"id":"490107","messageId":"3ef0927f-4d7b-4061-925e-c113d1c8730d@web.de","threadId":"60991","inReplyTo":"0947cb09-8b07-4fcd-bbe2-ae37c2cd5ec7@gmail.com","subject":"Re: [PATCH v2 4/4] t-ctype: avoid duplicating class names","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-06T18:16:37Z","receivedAt":"2024-03-06T18:17:06Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Hello Phillip,\n\nAm 04.03.24 um 10:51 schrieb Phillip Wood:\n> On 03/03/2024 10:13, René Scharfe wrote:\n>> TEST_CTYPE_FUNC defines a function for testing a character classifier,\n>> TEST_CHAR_CLASS calls it, causing the class name to be mentioned twice.\n>>\n>> Avoid the need to define a class-specific function by letting\n>> TEST_CHAR_CLASS do all the work.  This is done by using the internal\n>> functions test__run_begin() and test__run_end(), but they do exist to be\n>> used in test macros after all.\n>\n> Those internal functions exist to implement the TEST() macro, they\n> are not really intended for use outside that (which is why they are\n> marked as private in the header file). If we ever want to update the\n> implementation of TEST() it will be a lot harder if we're using the\n> internal implementation directly in test files. Unit tests should be\n> wrapping TEST() if it is appropriate but not the internal\n> implementation directly.\n\nforcing tests to be expressions and not allow them to use statements is\nan unusual requirement.  I don't see how the added friction would make\ntests any better.  It just requires more boilerplate code and annoying\nrepetition.  What kind of changes do you envision that would be\nhindered by allowing statements?\n\n> Ideally we wouldn't need TEST_CTYPE_FUNC as there would only be a\n> single function that was passed a ctype predicate, an input array and\n> an array of expected results. Unfortunately I don't think that is\n> possible due the the way the ctype predicates are implemented. Having\n> separate macros to define the test function and to run the test is\n> annoying but I don't think it is really worth exposing the internal\n> implementation just to avoid it.\n\nThe classifiers are currently implemented as macros.  We could turn them\ninto inline functions and would then be able to pass them to a test\nfunction.  Improving testability is a good idea, but also somehow feels\nlike the tail wagging the dog.  It would be easy, though, I think.  And\nless gross than:\n\n>> Alternatively we could unroll the loop to provide a very long expression\n>> that tests all 256 characters and EOF and hand that to TEST, but that\n>> seems awkward and hard to read.\n\n... which would yield unsightly test macros and huge test binaries.  But\nit would certainly be possible, and keep the definitions of the actual\ntests clean.\n\nRené\n"},{"id":"490109","messageId":"983c9e1f-2981-4e57-b00b-7ee5fe080b90@web.de","threadId":"60991","inReplyTo":"CAP8UFD2M2+x-pRX2nzCXbLW=nKcW4_RWc9qua5q-fU8QbGu1oA@mail.gmail.com","subject":"Re: [PATCH 3/3] t-ctype: do one test per class and char","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-06T18:16:44Z","receivedAt":"2024-03-06T18:21:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 04.03.24 um 11:00 schrieb Christian Couder:\n> And anyway it doesn't make sense to have meaningful messages as second\n> arguments to the TEST() macros if we always want to use a test harness\n> that just discards them. Either:\n>\n>   - we decide that we will always use some test harness, and then we\n> might just want to remove that second argument and yeah we can have\n> thousands of tests output lines from a single binary, or\n>   - we acknowledge that we don't always want to use a test harness,\n> and then we want a relatively short and meaningful output from a\n> single binary.\n\nDifferent situations require different levels of detail.  If all checks\npass, we just need that one bit of information.  If some fail, we need\nas much helpful context as we can get.  Success messages are interesting\nfor someone who added a new test and as a progress indicator, but are\nuseless otherwise (to me at least).\n\nPerhaps it would make sense to show ok message only after a delay if\nwriting to a terminal, similar to what progress.c does?  This would\neffectively silence them, since our current unit tests currently take\nonly a fraction of a second to finish.\n\nRené\n"},{"id":"490243","messageId":"f2b5914b-e26e-4d92-b119-0e58ce901bb9@gmail.com","threadId":"60991","inReplyTo":"3ef0927f-4d7b-4061-925e-c113d1c8730d@web.de","subject":"Re: [PATCH v2 4/4] t-ctype: avoid duplicating class names","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-03-08T14:05:30Z","receivedAt":"2024-03-08T14:05:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 06/03/2024 18:16, René Scharfe wrote:\n\nSorry for the delay I somehow missed your reply\n\n> Hello Phillip,\n> \n> Am 04.03.24 um 10:51 schrieb Phillip Wood:\n>> On 03/03/2024 10:13, René Scharfe wrote:\n>>> TEST_CTYPE_FUNC defines a function for testing a character classifier,\n>>> TEST_CHAR_CLASS calls it, causing the class name to be mentioned twice.\n>>>\n>>> Avoid the need to define a class-specific function by letting\n>>> TEST_CHAR_CLASS do all the work.  This is done by using the internal\n>>> functions test__run_begin() and test__run_end(), but they do exist to be\n>>> used in test macros after all.\n>>\n>> Those internal functions exist to implement the TEST() macro, they\n>> are not really intended for use outside that (which is why they are\n>> marked as private in the header file). If we ever want to update the\n>> implementation of TEST() it will be a lot harder if we're using the\n>> internal implementation directly in test files. Unit tests should be\n>> wrapping TEST() if it is appropriate but not the internal\n>> implementation directly.\n> \n> forcing tests to be expressions and not allow them to use statements is\n> an unusual requirement.\n\nI don't think it is that unusual to require tests to be implemented as \nfunctions which more or less amounts to the same thing.\n\n> I don't see how the added friction would make\n> tests any better.  It just requires more boilerplate code and annoying\n> repetition.  What kind of changes do you envision that would be\n> hindered by allowing statements?\n\nI'm worried about bugs being introduced by the internal functions being \nused incorrectly - it is not a user friendly API because it is designed \naround the limitations of implementing TEST(), not for general \nconsumption. The unit test framework is very new so I don't think we can \nbe sure that we wont need to change it and that will be more difficult \nif unit tests do not use TEST(). Maybe one of the changes we need is a \nbetter way of allowing statements?\n\n>> Ideally we wouldn't need TEST_CTYPE_FUNC as there would only be a\n>> single function that was passed a ctype predicate, an input array and\n>> an array of expected results. Unfortunately I don't think that is\n>> possible due the the way the ctype predicates are implemented. Having\n>> separate macros to define the test function and to run the test is\n>> annoying but I don't think it is really worth exposing the internal\n>> implementation just to avoid it.\n> \n> The classifiers are currently implemented as macros.  We could turn them\n> into inline functions and would then be able to pass them to a test\n> function.  Improving testability is a good idea, but also somehow feels\n> like the tail wagging the dog.  It would be easy, though, I think.  And\n> less gross than:\n\nMaking them functions would allow them to be passed as function \narguments in our code as well, though I don't know if we have much use \nfor that. I certainly agree it would be better than the alternative below.\n\nBest Wishes\n\nPhillip\n\n>>> Alternatively we could unroll the loop to provide a very long expression\n>>> that tests all 256 characters and EOF and hand that to TEST, but that\n>>> seems awkward and hard to read.\n> \n> ... which would yield unsightly test macros and huge test binaries.  But\n> it would certainly be possible, and keep the definitions of the actual\n> tests clean.\n> \n> René\n> \n\n"},{"id":"490291","messageId":"4f41f509-1d44-4476-92b0-9bb643f64576@gmail.com","threadId":"60991","inReplyTo":"3ef0927f-4d7b-4061-925e-c113d1c8730d@web.de","subject":"Re: [PATCH v2 4/4] t-ctype: avoid duplicating class names","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-03-09T11:28:49Z","receivedAt":"2024-03-09T11:28:52Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi René\n\nOn 06/03/2024 18:16, René Scharfe wrote:\n> Hello Phillip,\n> \n> Am 04.03.24 um 10:51 schrieb Phillip Wood:\n>> On 03/03/2024 10:13, René Scharfe wrote:\n>>> TEST_CTYPE_FUNC defines a function for testing a character classifier,\n>>> TEST_CHAR_CLASS calls it, causing the class name to be mentioned twice.\n>>>\n>>> Avoid the need to define a class-specific function by letting\n>>> TEST_CHAR_CLASS do all the work.  This is done by using the internal\n>>> functions test__run_begin() and test__run_end(), but they do exist to be\n>>> used in test macros after all.\n>>\n>> Those internal functions exist to implement the TEST() macro, they\n>> are not really intended for use outside that (which is why they are\n>> marked as private in the header file). If we ever want to update the\n>> implementation of TEST() it will be a lot harder if we're using the\n>> internal implementation directly in test files. Unit tests should be\n>> wrapping TEST() if it is appropriate but not the internal\n>> implementation directly.\n> \n> forcing tests to be expressions and not allow them to use statements is\n> an unusual requirement.  I don't see how the added friction would make\n> tests any better.  It just requires more boilerplate code and annoying\n> repetition.  What kind of changes do you envision that would be\n> hindered by allowing statements?\n\nOn reflection I don't think I'm objecting to allowing statements, only \nthe use of the private functions to do so. If we tweak test__run_begin() \nand test__run_end() so that the description is passed to \ntest__run_begin() and we invert the return value of that function to \nmatch what test__run_end() is expecting then we can have\n\n#define TEST_BEGIN(...)\t\t\t\t\t\t\\\n\tdo {\t\t\t\t\t\t\t\\\n\t\tint run__ = test__run_begin(__VA_ARGS__);\t\\\n\t\tif (run__)\n\n#define TEST_END\t\t\t\t\\\n\t\ttest_run_end(run__);\t\t\\\n\t} while (0)\n\nWhich allow test authors to write\n\n\tTEST_BEGIN(\"my test\") {\n\t\t/* test body here */\n\t} TEST_END;\n\nThe macros insulate the test code from having to worry about \ntest_skip_all() and the \"do { ... } while (0)\" means that the compiler \nwill complain if the author forgets TEST_END. I'm slightly on the fence \nabout including the braces in the macros instead as that would make them \nharder to misuse but it would be less obvious that the test body is run \nin its own block. The compiler will allow the test author to \naccidentally nest two calls to TEST_BEGIN() but there is an assertion in \ntest__run_begin() which will catch that at run time.\n\nThe slight downside compared to TEST() is that it is harder to return \nearly if a check fails - we'd need something like\n\n\tTEST_BEGIN(\"my test\") {\n\t\tif (!check(0))\n\t\t\tgoto fail\n\t\t/* more checks */\n\t fail:\n\t\t;\n\t} TEST_END;\n\nAlso unlike TEST(), TEST_END does not indicate to the caller whether the \ntest failed or not but I'm not sure that matters in practice.\n\nWhat do you think?\n\nBest Wishes\n\nPhillip\n\n\n>> Ideally we wouldn't need TEST_CTYPE_FUNC as there would only be a\n>> single function that was passed a ctype predicate, an input array and\n>> an array of expected results. Unfortunately I don't think that is\n>> possible due the the way the ctype predicates are implemented. Having\n>> separate macros to define the test function and to run the test is\n>> annoying but I don't think it is really worth exposing the internal\n>> implementation just to avoid it.\n> \n> The classifiers are currently implemented as macros.  We could turn them\n> into inline functions and would then be able to pass them to a test\n> function.  Improving testability is a good idea, but also somehow feels\n> like the tail wagging the dog.  It would be easy, though, I think.  And\n> less gross than:\n> \n>>> Alternatively we could unroll the loop to provide a very long expression\n>>> that tests all 256 characters and EOF and hand that to TEST, but that\n>>> seems awkward and hard to read.\n> \n> ... which would yield unsightly test macros and huge test binaries.  But\n> it would certainly be possible, and keep the definitions of the actual\n> tests clean.\n> \n> René\n> \n\n"},{"id":"490320","messageId":"69e24019-1fff-40df-9aef-cdfd058124c9@web.de","threadId":"60991","inReplyTo":"4f41f509-1d44-4476-92b0-9bb643f64576@gmail.com","subject":"Re: [PATCH v2 4/4] t-ctype: avoid duplicating class names","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-10T12:48:22Z","receivedAt":"2024-03-10T12:48:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Hi Phillip,\n\nAm 09.03.24 um 12:28 schrieb Phillip Wood:\n> On reflection I don't think I'm objecting to allowing statements,\n> only the use of the private functions to do so. If we tweak\n> test__run_begin() and test__run_end() so that the description is\n> passed to test__run_begin() and we invert the return value of that\n> function to match what test__run_end() is expecting then we can have\n>\n> #define TEST_BEGIN(...)                        \\\n>     do {                            \\\n>         int run__ = test__run_begin(__VA_ARGS__);    \\\n>         if (run__)\n>\n> #define TEST_END                \\\n>         test_run_end(run__);        \\\n>     } while (0)\n>\n> Which allow test authors to write\n>\n>     TEST_BEGIN(\"my test\") {\n>         /* test body here */\n>     } TEST_END;\n>\n> The macros insulate the test code from having to worry about\n> test_skip_all() and the \"do { ... } while (0)\" means that the\n> compiler will complain if the author forgets TEST_END.\n\nthe location information is missing, but I get the idea.  That would\ncertainly work for t-ctype.\n\n> I'm slightly on the fence about including the braces in the macros\n> instead as that would make them harder to misuse but it would be less\n> obvious that the test body is run in its own block. The compiler will\n> allow the test author to accidentally nest two calls to TEST_BEGIN()\n> but there is an assertion in test__run_begin() which will catch that\n> at run time.\n\nThought about that as well, and I'd also be wary of including any of the\ncontrol statements.  Custom syntax requires learning, can have weird\nside-effects and may be misunderstood by editors.\n\nBelow is a patch that adds the function test_start() and its companion\nmacro TEST_START, which allow defining tests with minimal ceremony,\nsimilarly as in the shell-based test suite:\n\n\t#include \"test-lib.h\"\n\n\tint cmd_main(int argc, const char **argv)\n\t{\n\t\tif (TEST_START(\"something works\"))\n\t\t\tcheck(something());\n\t\tif (TEST_START(\"something else works\"))\n\t\t\tcheck(something_else());\n\t\treturn test_done();\n\t}\n\nIt requires storing string copies and sits between the states of the\noriginal test machinery, so it's a bit complicated.\n\nThe biggest downside so far, though, is that I couldn't find an example\nin the other unit tests that it would simplify significantly.  At least\nit would allow getting rid of the void pointers in t-strbuf, however.\n\n> The slight downside compared to TEST() is that it is harder to return\n> early if a check fails - we'd need something like\n>\n>     TEST_BEGIN(\"my test\") {\n>         if (!check(0))\n>             goto fail\n>         /* more checks */\n>      fail:\n>         ;\n>     } TEST_END;\n\nTEST is worse in this regard, as it doesn't allow \"if\" directly at all.\nYou can use short-circuiting, of course:\n\n\tTEST(check(!!ptr) && check(*ptr == value), \"ptr points to value\");\n\nBut you can do that with TEST_BEGIN as well.  In a function you can\nreturn early, but you can use functions with both, too.\n\nIn your example you can use \"continue\" instead of \"goto fail\".  And with\n\"break\" you can skip the test_run_end() call.  I consider both to be\ndownsides, though -- the abstraction is leaky.\n\n> Also unlike TEST(), TEST_END does not indicate to the caller whether\n> the test failed or not but I'm not sure that matters in practice.\n\nMost TEST invocations out of t-basic ignore its return value so far.\n\nctx.result is left unchanged by test__run_end(), so we could still\naccess it if really needed.  Perhaps through a sanctioned function,\nlast_test_result() or similar.\n\nRené\n\n\n\ndiff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\nindex d6ac1fe678..e7c846814e 100644\n--- a/t/unit-tests/t-ctype.c\n+++ b/t/unit-tests/t-ctype.c\n@@ -4,15 +4,13 @@\n \tsize_t len = ARRAY_SIZE(string) - 1 + \\\n \t\tBUILD_ASSERT_OR_ZERO(ARRAY_SIZE(string) > 0) + \\\n \t\tBUILD_ASSERT_OR_ZERO(sizeof(string[0]) == sizeof(char)); \\\n-\tint skip = test__run_begin(); \\\n-\tif (!skip) { \\\n+\tif (TEST_START(#class \" works\")) { \\\n \t\tfor (int i = 0; i < 256; i++) { \\\n \t\t\tif (!check_int(class(i), ==, !!memchr(string, i, len)))\\\n \t\t\t\ttest_msg(\"      i: 0x%02x\", i); \\\n \t\t} \\\n \t\tcheck(!class(EOF)); \\\n \t} \\\n-\ttest__run_end(!skip, TEST_LOCATION(), #class \" works\"); \\\n } while (0)\n\n #define DIGIT \"0123456789\"\ndiff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c\nindex 66d6980ffb..bc484c92da 100644\n--- a/t/unit-tests/test-lib.c\n+++ b/t/unit-tests/test-lib.c\n@@ -16,6 +16,8 @@ static struct {\n \tunsigned running :1;\n \tunsigned skip_all :1;\n \tunsigned todo :1;\n+\tchar *desc;\n+\tchar *location;\n } ctx = {\n \t.lazy_plan = 1,\n \t.result = RESULT_NONE,\n@@ -123,9 +125,45 @@ void test_plan(int count)\n \tctx.lazy_plan = 0;\n }\n\n+static void test_run_maybe_end(void)\n+{\n+\tif (ctx.running) {\n+\t\tassert(ctx.location);\n+\t\tassert(ctx.desc);\n+\t\ttest__run_end(0, ctx.location, \"%s\", ctx.desc);\n+\t\tFREE_AND_NULL(ctx.location);\n+\t\tFREE_AND_NULL(ctx.desc);\n+\t}\n+\tassert(!ctx.running);\n+\tassert(!ctx.location);\n+\tassert(!ctx.desc);\n+}\n+\n+int test_start(const char *location, const char *format, ...)\n+{\n+\tva_list ap;\n+\tchar *desc;\n+\n+\ttest_run_maybe_end();\n+\n+\tva_start(ap, format);\n+\tdesc = xstrvfmt(format, ap);\n+\tva_end(ap);\n+\n+\tif (test__run_begin()) {\n+\t\ttest__run_end(1, location, \"%s\", desc);\n+\t\tfree(desc);\n+\t\treturn 0;\n+\t} else {\n+\t\tctx.location = xstrdup(location);\n+\t\tctx.desc = desc;\n+\t\treturn 1;\n+\t}\n+}\n+\n int test_done(void)\n {\n-\tassert(!ctx.running);\n+\ttest_run_maybe_end();\n\n \tif (ctx.lazy_plan)\n \t\ttest_plan(ctx.count);\ndiff --git a/t/unit-tests/test-lib.h b/t/unit-tests/test-lib.h\nindex a8f07ae0b7..2be95b3ab8 100644\n--- a/t/unit-tests/test-lib.h\n+++ b/t/unit-tests/test-lib.h\n@@ -21,6 +21,15 @@\n  */\n void test_plan(int count);\n\n+/*\n+ * Start a test.  It ends when the next test starts or test_done()\n+ * is called.  Returns 1 if the test was actually started, 0 if it was\n+ * skipped because test_skip_all() had been called.\n+ */\n+int test_start(const char *location, const char *format, ...);\n+\n+#define TEST_START(...) test_start(TEST_LOCATION(), __VA_ARGS__)\n+\n /*\n  * test_done() must be called at the end of main(). It will print the\n  * plan if plan() was not called at the beginning of the test program\n"}]}