diff mbox series

[1/3] t-ctype: allow NUL anywhere in the specification string

Message ID 20240225112722.89221-2-l.s.r@web.de (mailing list archive)
State New, archived
Headers show
Series t-ctype: simplify unit test definitions | expand

Commit Message

René Scharfe Feb. 25, 2024, 11:27 a.m. UTC 
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

Comments

Eric Sunshine Feb. 25, 2024, 6:05 p.m. UTC | #1 
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))) \
René Scharfe Feb. 25, 2024, 6:28 p.m. UTC | #2 
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))) \
Eric Sunshine Feb. 25, 2024, 6:41 p.m. UTC | #3 
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/
Jeff King Feb. 25, 2024, 9 p.m. UTC | #4 
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
Eric Sunshine Feb. 25, 2024, 9:02 p.m. UTC | #5 
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 mbox series

Patch

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))) \