| Message ID | 20210507050908.1008686-2-davidgow@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | [1/2] kunit: Do not typecheck binary assertions | expand |
On Thu, May 6, 2021 at 10:09 PM David Gow <davidgow@google.com> wrote: > > Currently, the KUNIT_EXPECT_STREQ() and related macros assign both > string arguments to variables of their own type (via typeof()). This > seems to be to prevent the macro argument from being evaluated multiple > times. > > However, yhis doesn't work if one of these is a fixed-length character nit: if you ever send a v2 of this patch, s/yhis/this > array, rather than a character pointer, as (for example) char[16] will > always allocate a new string. > > By always using 'const char*' (the type strcmp expects), we're always > just taking a pointer to the string, which works even with character > arrays. > > Signed-off-by: David Gow <davidgow@google.com> Reviewed-by: Daniel Latypov <dlatypov@google.com> I'm very happy to see this patch. This makes code that looks obviously correct actually work. Somewhat tangential: there are several casts that are no longer needed after this in the docs. I think the following gets rid of all of them. Should it perhaps go in a chain with this patch? I.e. if the first one is too controversial and we want to go ahead split this patch off from it. diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst index 650f99590df5..756747417a19 100644 --- a/Documentation/dev-tools/kunit/usage.rst +++ b/Documentation/dev-tools/kunit/usage.rst @@ -465,10 +465,9 @@ fictitious example for ``sha1sum(1)`` .. code-block:: c - /* Note: the cast is to satisfy overly strict type-checking. */ #define TEST_SHA1(in, want) \ sha1sum(in, out); \ - KUNIT_EXPECT_STREQ_MSG(test, (char *)out, want, "sha1sum(%s)", in); + KUNIT_EXPECT_STREQ_MSG(test, out, want, "sha1sum(%s)", in); char out[40]; TEST_SHA1("hello world", "2aae6c35c94fcfb415dbe95f408b9ce91ee846ed"); @@ -507,7 +506,7 @@ In some cases, it can be helpful to write a *table-driven test* instead, e.g. }; for (i = 0; i < ARRAY_SIZE(cases); ++i) { sha1sum(cases[i].str, out); - KUNIT_EXPECT_STREQ_MSG(test, (char *)out, cases[i].sha1, + KUNIT_EXPECT_STREQ_MSG(test, out, cases[i].sha1, "sha1sum(%s)", cases[i].str); } @@ -568,7 +567,7 @@ Reusing the same ``cases`` array from above, we can write the test as a struct sha1_test_case *test_param = (struct sha1_test_case *)(test->param_value); sha1sum(test_param->str, out); - KUNIT_EXPECT_STREQ_MSG(test, (char *)out, test_param->sha1, + KUNIT_EXPECT_STREQ_MSG(test, out, test_param->sha1, "sha1sum(%s)", test_param->str); } > --- > include/kunit/test.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/kunit/test.h b/include/kunit/test.h > index 4c56ffcb7403..b68c61348121 100644 > --- a/include/kunit/test.h > +++ b/include/kunit/test.h > @@ -1128,8 +1128,8 @@ do { \ > fmt, \ > ...) \ > do { \ > - typeof(left) __left = (left); \ > - typeof(right) __right = (right); \ > + const char *__left = (left); \ > + const char *__right = (right); \ > \ > KUNIT_ASSERTION(test, \ > strcmp(__left, __right) op 0, \ > -- > 2.31.1.607.g51e8a6a459-goog >
On Thu, May 6, 2021 at 10:09 PM David Gow <davidgow@google.com> wrote: > > Currently, the KUNIT_EXPECT_STREQ() and related macros assign both > string arguments to variables of their own type (via typeof()). This > seems to be to prevent the macro argument from being evaluated multiple > times. > > However, yhis doesn't work if one of these is a fixed-length character > array, rather than a character pointer, as (for example) char[16] will > always allocate a new string. > > By always using 'const char*' (the type strcmp expects), we're always > just taking a pointer to the string, which works even with character > arrays. > > Signed-off-by: David Gow <davidgow@google.com> Aside from the nit that Daniel pointed out, this looks good to me. Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
diff --git a/include/kunit/test.h b/include/kunit/test.h index 4c56ffcb7403..b68c61348121 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -1128,8 +1128,8 @@ do { \ fmt, \ ...) \ do { \ - typeof(left) __left = (left); \ - typeof(right) __right = (right); \ + const char *__left = (left); \ + const char *__right = (right); \ \ KUNIT_ASSERTION(test, \ strcmp(__left, __right) op 0, \
Currently, the KUNIT_EXPECT_STREQ() and related macros assign both string arguments to variables of their own type (via typeof()). This seems to be to prevent the macro argument from being evaluated multiple times. However, yhis doesn't work if one of these is a fixed-length character array, rather than a character pointer, as (for example) char[16] will always allocate a new string. By always using 'const char*' (the type strcmp expects), we're always just taking a pointer to the string, which works even with character arrays. Signed-off-by: David Gow <davidgow@google.com> --- include/kunit/test.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)