* [PATCH 1/2] kunit: Do not typecheck binary assertions @ 2021-05-07 5:09 David Gow 2021-05-07 5:09 ` [PATCH 2/2] kunit: Assign strings to 'const char*' in STREQ assertions David Gow ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: David Gow @ 2021-05-07 5:09 UTC (permalink / raw) To: Brendan Higgins, Daniel Latypov, Shuah Khan Cc: David Gow, kunit-dev, linux-kselftest, linux-kernel The use of typecheck() in KUNIT_EXPECT_EQ() and friends is causing more problems than I think it's worth. Things like enums need to have their values explicitly cast, and literals all need to be very precisely typed for the code to compile. While typechecking does have its uses, the additional overhead of having lots of needless casts -- combined with the awkward error messages which don't mention which types are involved -- makes tests less readable and more difficult to write. By removing the typecheck() call, the two arguments still need to be of compatible types, but don't need to be of exactly the same time, which seems a less confusing and more useful compromise. Signed-off-by: David Gow <davidgow@google.com> --- I appreciate that this is probably a bit controversial (and, indeed, I was a bit hesitant about sending it out myself), but after sitting on it for a few days, I still think this is probably an improvement overall. The second patch does fix what I think is an actual bug, though, so even if this isn't determined to be a good idea, it (or some equivalent) should probably go through. Cheers, -- David include/kunit/test.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/kunit/test.h b/include/kunit/test.h index 49601c4b98b8..4c56ffcb7403 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -775,7 +775,6 @@ void kunit_do_assertion(struct kunit *test, do { \ typeof(left) __left = (left); \ typeof(right) __right = (right); \ - ((void)__typecheck(__left, __right)); \ \ KUNIT_ASSERTION(test, \ __left op __right, \ -- 2.31.1.607.g51e8a6a459-goog ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] kunit: Assign strings to 'const char*' in STREQ assertions 2021-05-07 5:09 [PATCH 1/2] kunit: Do not typecheck binary assertions David Gow @ 2021-05-07 5:09 ` David Gow 2021-05-07 7:28 ` Daniel Latypov 2021-05-07 20:07 ` Brendan Higgins 2021-05-07 7:25 ` [PATCH 1/2] kunit: Do not typecheck binary assertions Daniel Latypov 2021-05-07 20:05 ` Brendan Higgins 2 siblings, 2 replies; 7+ messages in thread From: David Gow @ 2021-05-07 5:09 UTC (permalink / raw) To: Brendan Higgins, Daniel Latypov, Shuah Khan Cc: David Gow, kunit-dev, linux-kselftest, linux-kernel 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(-) 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] kunit: Assign strings to 'const char*' in STREQ assertions 2021-05-07 5:09 ` [PATCH 2/2] kunit: Assign strings to 'const char*' in STREQ assertions David Gow @ 2021-05-07 7:28 ` Daniel Latypov 2021-05-07 20:07 ` Brendan Higgins 1 sibling, 0 replies; 7+ messages in thread From: Daniel Latypov @ 2021-05-07 7:28 UTC (permalink / raw) To: David Gow Cc: Brendan Higgins, Shuah Khan, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Linux Kernel Mailing List 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 > ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] kunit: Assign strings to 'const char*' in STREQ assertions 2021-05-07 5:09 ` [PATCH 2/2] kunit: Assign strings to 'const char*' in STREQ assertions David Gow 2021-05-07 7:28 ` Daniel Latypov @ 2021-05-07 20:07 ` Brendan Higgins 1 sibling, 0 replies; 7+ messages in thread From: Brendan Higgins @ 2021-05-07 20:07 UTC (permalink / raw) To: David Gow Cc: Daniel Latypov, Shuah Khan, kunit-dev, linux-kselftest, linux-kernel 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] kunit: Do not typecheck binary assertions 2021-05-07 5:09 [PATCH 1/2] kunit: Do not typecheck binary assertions David Gow 2021-05-07 5:09 ` [PATCH 2/2] kunit: Assign strings to 'const char*' in STREQ assertions David Gow @ 2021-05-07 7:25 ` Daniel Latypov 2021-05-07 20:05 ` Brendan Higgins 2 siblings, 0 replies; 7+ messages in thread From: Daniel Latypov @ 2021-05-07 7:25 UTC (permalink / raw) To: David Gow Cc: Brendan Higgins, Shuah Khan, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Linux Kernel Mailing List On Thu, May 6, 2021 at 10:09 PM David Gow <davidgow@google.com> wrote: > > The use of typecheck() in KUNIT_EXPECT_EQ() and friends is causing more > problems than I think it's worth. Things like enums need to have their > values explicitly cast, and literals all need to be very precisely typed > for the code to compile. > > While typechecking does have its uses, the additional overhead of having > lots of needless casts -- combined with the awkward error messages which > don't mention which types are involved -- makes tests less readable and > more difficult to write. > > By removing the typecheck() call, the two arguments still need to be of > compatible types, but don't need to be of exactly the same time, which > seems a less confusing and more useful compromise. > > Signed-off-by: David Gow <davidgow@google.com> > --- > > I appreciate that this is probably a bit controversial (and, indeed, I > was a bit hesitant about sending it out myself), but after sitting on it > for a few days, I still think this is probably an improvement overall. I'm in favor. The absolute worst part of the status quo is that the types involved might not get shown at all in the GCC error output! It's an incredible pain and probably has wasted a good deal of other people's time as well. (Maybe clang is better in this regard). Here's a few examples where things get a bit weird: KUNIT_EXPECT_EQ(test, 1 + 1, 2.5); Expected 1 + 1 == 2.5, but 1 + 1 == 2 2.5 == 2 Along similar lines: KUNIT_EXPECT_EQ(test, 0xffffffff, ~0); KUNIT_EXPECT_EQ(test, 0xffffffffffffffff, ~0); KUNIT_EXPECT_EQ(test, 0xfffffffffffffffe, ~1); KUNIT_EXPECT_EQ(test, 0xfffffffe, ~0); //fails The failure message on the last might make one wonder how the first ones worked. Expected 0xfffffffe == ~0, but 0xfffffffe == 4294967294 ~0 == -1 Explanation: when evaluating the assertion, we compare __left/__right directly which maintain their types. But struct kunit_binary_assert stores them as `long long`, hence the truncation of 2.5 to 2. I was nervous about ~0, as it should be an int, i.e. this passes: KUNIT_EXPECT_EQ(test, sizeof(~0), sizeof(int)) But it all works as expected, e.g. we don't have implicit narrowing going on and causing us to say that 0xfffffffffffffffe = 0. Stuff like KUNIT_EXPECT_EQ(test, 0, NULL); will compile, but with warnings ../include/kunit/test.h:805:15: warning: comparison between pointer and integer 805 | left, ==, right, \ | ^~ So I generally think that we can rely on compiler warnings to protect us from some misuse. Reviewed-by: Daniel Latypov <dlatypov@google.com> > > The second patch does fix what I think is an actual bug, though, so even > if this isn't determined to be a good idea, it (or some equivalent) > should probably go through. > > Cheers, > -- David > > include/kunit/test.h | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/include/kunit/test.h b/include/kunit/test.h > index 49601c4b98b8..4c56ffcb7403 100644 > --- a/include/kunit/test.h > +++ b/include/kunit/test.h > @@ -775,7 +775,6 @@ void kunit_do_assertion(struct kunit *test, > do { \ > typeof(left) __left = (left); \ > typeof(right) __right = (right); \ > - ((void)__typecheck(__left, __right)); \ > \ > KUNIT_ASSERTION(test, \ > __left op __right, \ > -- > 2.31.1.607.g51e8a6a459-goog > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] kunit: Do not typecheck binary assertions 2021-05-07 5:09 [PATCH 1/2] kunit: Do not typecheck binary assertions David Gow 2021-05-07 5:09 ` [PATCH 2/2] kunit: Assign strings to 'const char*' in STREQ assertions David Gow 2021-05-07 7:25 ` [PATCH 1/2] kunit: Do not typecheck binary assertions Daniel Latypov @ 2021-05-07 20:05 ` Brendan Higgins 2021-05-08 5:56 ` David Gow 2 siblings, 1 reply; 7+ messages in thread From: Brendan Higgins @ 2021-05-07 20:05 UTC (permalink / raw) To: David Gow Cc: Daniel Latypov, Shuah Khan, kunit-dev, linux-kselftest, linux-kernel On Thu, May 6, 2021 at 10:09 PM David Gow <davidgow@google.com> wrote: > > The use of typecheck() in KUNIT_EXPECT_EQ() and friends is causing more > problems than I think it's worth. Things like enums need to have their > values explicitly cast, and literals all need to be very precisely typed > for the code to compile. nit: I have not had the typecheck() call prevent any code from compiling, just generating warnings. I guess you can have a build set to cause any warning to be promoted to an error; still, I think this statement is misleading. > While typechecking does have its uses, the additional overhead of having > lots of needless casts -- combined with the awkward error messages which > don't mention which types are involved -- makes tests less readable and > more difficult to write. > > By removing the typecheck() call, the two arguments still need to be of > compatible types, but don't need to be of exactly the same time, which > seems a less confusing and more useful compromise. > > Signed-off-by: David Gow <davidgow@google.com> Looks good to me. Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > --- > > I appreciate that this is probably a bit controversial (and, indeed, I > was a bit hesitant about sending it out myself), but after sitting on it > for a few days, I still think this is probably an improvement overall. > > The second patch does fix what I think is an actual bug, though, so even > if this isn't determined to be a good idea, it (or some equivalent) > should probably go through. I don't remember being a huge fan of the typecheck when it was asked for either. I think I am a little bit more indifferent than you; nevertheless, I support this change. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] kunit: Do not typecheck binary assertions 2021-05-07 20:05 ` Brendan Higgins @ 2021-05-08 5:56 ` David Gow 0 siblings, 0 replies; 7+ messages in thread From: David Gow @ 2021-05-08 5:56 UTC (permalink / raw) To: Brendan Higgins Cc: Daniel Latypov, Shuah Khan, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Linux Kernel Mailing List On Sat, May 8, 2021 at 4:05 AM 'Brendan Higgins' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > On Thu, May 6, 2021 at 10:09 PM David Gow <davidgow@google.com> wrote: > > > > The use of typecheck() in KUNIT_EXPECT_EQ() and friends is causing more > > problems than I think it's worth. Things like enums need to have their > > values explicitly cast, and literals all need to be very precisely typed > > for the code to compile. > > nit: I have not had the typecheck() call prevent any code from > compiling, just generating warnings. I guess you can have a build set > to cause any warning to be promoted to an error; still, I think this > statement is misleading. > Whoops -- it was the issue in patch 2 that was causing the error. This is indeed just a warning. I'll send out a second version with a more accurate description next week, assuming no further issues appear. > > While typechecking does have its uses, the additional overhead of having > > lots of needless casts -- combined with the awkward error messages which > > don't mention which types are involved -- makes tests less readable and > > more difficult to write. > > > > By removing the typecheck() call, the two arguments still need to be of > > compatible types, but don't need to be of exactly the same time, which > > seems a less confusing and more useful compromise. > > > > Signed-off-by: David Gow <davidgow@google.com> > > Looks good to me. > > Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > > > --- > > > > I appreciate that this is probably a bit controversial (and, indeed, I > > was a bit hesitant about sending it out myself), but after sitting on it > > for a few days, I still think this is probably an improvement overall. > > > > The second patch does fix what I think is an actual bug, though, so even > > if this isn't determined to be a good idea, it (or some equivalent) > > should probably go through. > > I don't remember being a huge fan of the typecheck when it was asked > for either. I think I am a little bit more indifferent than you; > nevertheless, I support this change. > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/CAFd5g44bot7S-Ya7s7QxnKfXHcy8WxUcNPsZuw_qWMaAQbqLCg%40mail.gmail.com. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-05-08 5:57 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-07 5:09 [PATCH 1/2] kunit: Do not typecheck binary assertions David Gow 2021-05-07 5:09 ` [PATCH 2/2] kunit: Assign strings to 'const char*' in STREQ assertions David Gow 2021-05-07 7:28 ` Daniel Latypov 2021-05-07 20:07 ` Brendan Higgins 2021-05-07 7:25 ` [PATCH 1/2] kunit: Do not typecheck binary assertions Daniel Latypov 2021-05-07 20:05 ` Brendan Higgins 2021-05-08 5:56 ` David Gow
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).