| Message ID | 20240225112722.89221-2-l.s.r@web.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | t-ctype: simplify unit test definitions | expand |
On Sun, Feb 25, 2024 at 6:27 AM René Scharfe <l.s.r@web.de> wrote: > Replace the custom function is_in() for looking up a character in the > specification string with memchr(3) and sizeof. This is shorter, > simpler and allows NUL anywhere in the string, which may come in handy > if we ever want to support more character classes that contain it. > > Getting the string size using sizeof only works in a macro and with a > string constant, but that's exactly what we have and I don't see it > changing anytime soon. > > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c > @@ -1,23 +1,11 @@ > /* Macro to test a character type */ > #define TEST_CTYPE_FUNC(func, string) \ Taking into consideration the commit message warning about string constants, would it make sense to update the comment to mention that limitation? /* Test a character type. (Only use with string constants.) */ #define TEST_CTYPE_FUNC(func, string) \ > static void test_ctype_##func(void) { \ > for (int i = 0; i < 256; i++) { \ > - if (!check_int(func(i), ==, is_in(string, i))) \ > + int expect = !!memchr(string, i, sizeof(string) - 1); \ > + if (!check_int(func(i), ==, expect)) \ > test_msg(" i: 0x%02x", i); \ > } \ > if (!check(!func(EOF))) \
Am 25.02.24 um 19:05 schrieb Eric Sunshine: > On Sun, Feb 25, 2024 at 6:27 AM René Scharfe <l.s.r@web.de> wrote: >> Replace the custom function is_in() for looking up a character in the >> specification string with memchr(3) and sizeof. This is shorter, >> simpler and allows NUL anywhere in the string, which may come in handy >> if we ever want to support more character classes that contain it. >> >> Getting the string size using sizeof only works in a macro and with a >> string constant, but that's exactly what we have and I don't see it >> changing anytime soon. >> >> Signed-off-by: René Scharfe <l.s.r@web.de> >> --- >> diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c >> @@ -1,23 +1,11 @@ >> /* Macro to test a character type */ >> #define TEST_CTYPE_FUNC(func, string) \ > > Taking into consideration the commit message warning about string > constants, would it make sense to update the comment to mention that > limitation? I think the temptation to pass a string pointer is low -- if only because there aren't any in this file. But adding such a warning can't hurt, so yeah, why not? > > /* Test a character type. (Only use with string constants.) */ > #define TEST_CTYPE_FUNC(func, string) \ > >> static void test_ctype_##func(void) { \ >> for (int i = 0; i < 256; i++) { \ >> - if (!check_int(func(i), ==, is_in(string, i))) \ >> + int expect = !!memchr(string, i, sizeof(string) - 1); \ >> + if (!check_int(func(i), ==, expect)) \ >> test_msg(" i: 0x%02x", i); \ >> } \ >> if (!check(!func(EOF))) \
On Sun, Feb 25, 2024 at 1:28 PM René Scharfe <l.s.r@web.de> wrote: > Am 25.02.24 um 19:05 schrieb Eric Sunshine: > > On Sun, Feb 25, 2024 at 6:27 AM René Scharfe <l.s.r@web.de> wrote: > >> Getting the string size using sizeof only works in a macro and with a > >> string constant, but that's exactly what we have and I don't see it > >> changing anytime soon. > >> > >> /* Macro to test a character type */ > >> #define TEST_CTYPE_FUNC(func, string) \ > > > > Taking into consideration the commit message warning about string > > constants, would it make sense to update the comment to mention that > > limitation? > > > > /* Test a character type. (Only use with string constants.) */ > > #define TEST_CTYPE_FUNC(func, string) \ > >> static void test_ctype_##func(void) { \ > >> for (int i = 0; i < 256; i++) { \ > >> + int expect = !!memchr(string, i, sizeof(string) - 1); \ > > I think the temptation to pass a string pointer is low -- if only > because there aren't any in this file. But adding such a warning > can't hurt, so yeah, why not? The patch just posted[1] by SZEDER reminded me that, on this project, we assume that the compiler is smart enough to replace `strlen("string-literal")` with the constant `15`, so rather than worrying about updating comment to mention the sizeof() limitation, you could perhaps just use `strlen(string)` instead of `sizeof(string)-1`? [1]: https://lore.kernel.org/git/20240225183452.1939334-1-szeder.dev@gmail.com/
On Sun, Feb 25, 2024 at 01:41:30PM -0500, Eric Sunshine wrote: > On Sun, Feb 25, 2024 at 1:28 PM René Scharfe <l.s.r@web.de> wrote: > > Am 25.02.24 um 19:05 schrieb Eric Sunshine: > > > On Sun, Feb 25, 2024 at 6:27 AM René Scharfe <l.s.r@web.de> wrote: > > >> Getting the string size using sizeof only works in a macro and with a > > >> string constant, but that's exactly what we have and I don't see it > > >> changing anytime soon. > > >> > > >> /* Macro to test a character type */ > > >> #define TEST_CTYPE_FUNC(func, string) \ > > > > > > Taking into consideration the commit message warning about string > > > constants, would it make sense to update the comment to mention that > > > limitation? > > > > > > /* Test a character type. (Only use with string constants.) */ > > > #define TEST_CTYPE_FUNC(func, string) \ > > >> static void test_ctype_##func(void) { \ > > >> for (int i = 0; i < 256; i++) { \ > > >> + int expect = !!memchr(string, i, sizeof(string) - 1); \ > > > > I think the temptation to pass a string pointer is low -- if only > > because there aren't any in this file. But adding such a warning > > can't hurt, so yeah, why not? > > The patch just posted[1] by SZEDER reminded me that, on this project, > we assume that the compiler is smart enough to replace > `strlen("string-literal")` with the constant `15`, so rather than > worrying about updating comment to mention the sizeof() limitation, > you could perhaps just use `strlen(string)` instead of > `sizeof(string)-1`? That would defeat the advertised purpose that we can handle embedded NULs, though. Whereas with sizeof(), I think a literal like "foo\0bar" would still have length 8. -Peff
On Sun, Feb 25, 2024 at 4:00 PM Jeff King <peff@peff.net> wrote: > On Sun, Feb 25, 2024 at 01:41:30PM -0500, Eric Sunshine wrote: > > On Sun, Feb 25, 2024 at 1:28 PM René Scharfe <l.s.r@web.de> wrote: > > > Am 25.02.24 um 19:05 schrieb Eric Sunshine: > > > > Taking into consideration the commit message warning about string > > > > constants, would it make sense to update the comment to mention that > > > > limitation? > > > > > > I think the temptation to pass a string pointer is low -- if only > > > because there aren't any in this file. But adding such a warning > > > can't hurt, so yeah, why not? > > > > The patch just posted[1] by SZEDER reminded me that, on this project, > > we assume that the compiler is smart enough to replace > > `strlen("string-literal")` with the constant `15`, so rather than > > worrying about updating comment to mention the sizeof() limitation, > > you could perhaps just use `strlen(string)` instead of > > `sizeof(string)-1`? > > That would defeat the advertised purpose that we can handle embedded > NULs, though. Whereas with sizeof(), I think a literal like "foo\0bar" > would still have length 8. True. Sorry for the noise.
diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c index f315489984..64d7186258 100644 --- a/t/unit-tests/t-ctype.c +++ b/t/unit-tests/t-ctype.c @@ -1,23 +1,11 @@ #include "test-lib.h" -static int is_in(const char *s, int ch) -{ - /* - * We can't find NUL using strchr. Accept it as the first - * character in the spec -- there are no empty classes. - */ - if (ch == '\0') - return ch == *s; - if (*s == '\0') - s++; - return !!strchr(s, ch); -} - /* Macro to test a character type */ #define TEST_CTYPE_FUNC(func, string) \ static void test_ctype_##func(void) { \ for (int i = 0; i < 256; i++) { \ - if (!check_int(func(i), ==, is_in(string, i))) \ + int expect = !!memchr(string, i, sizeof(string) - 1); \ + if (!check_int(func(i), ==, expect)) \ test_msg(" i: 0x%02x", i); \ } \ if (!check(!func(EOF))) \
Replace the custom function is_in() for looking up a character in the specification string with memchr(3) and sizeof. This is shorter, simpler and allows NUL anywhere in the string, which may come in handy if we ever want to support more character classes that contain it. Getting the string size using sizeof only works in a macro and with a string constant, but that's exactly what we have and I don't see it changing anytime soon. Signed-off-by: René Scharfe <l.s.r@web.de> --- t/unit-tests/t-ctype.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) -- 2.44.0