Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210507050908.1008686-1-davidgow@google.com> <20210507050908.1008686-2-davidgow@google.com>
In-Reply-To: <20210507050908.1008686-2-davidgow@google.com>
From:   Daniel Latypov <dlatypov@google.com>
Date:   Fri, 7 May 2021 00:28:10 -0700
Message-ID: <CAGS_qxqOCUAKx7+0DBnsThVWskqPejq6oHzr4F+nvJALU+F1zw@mail.gmail.com>
Subject: Re: [PATCH 2/2] kunit: Assign strings to 'const char*' in STREQ assertions
To:     David Gow <davidgow@google.com>
Cc:     Brendan Higgins <brendanhiggins@google.com>,
        Shuah Khan <skhan@linuxfoundation.org>,
        KUnit Development <kunit-dev@googlegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>