[PATCH] Documentation: Kunit: Fix example with compilation error

[PATCH] Documentation: Kunit: Fix example with compilation error

[Maíra Canal]

The Parameterized Testing example contains a compilation error, as the
signature for the description helper function should be void(*)(struct
sha1_test_case *, char *), so the struct should not be const. This is
warned by Clang:

error: initialization of ‘void (*)(struct sha1_test_case *, char *)’
from incompatible pointer type ‘void (*)(const struct sha1_test_case *,
char *)’ [-Werror=incompatible-pointer-types]
    33 | KUNIT_ARRAY_PARAM(sha1, cases, case_to_desc);
       |                                ^~~~~~~~~~~~
../include/kunit/test.h:1339:70: note: in definition of macro
‘KUNIT_ARRAY_PARAM’
1339 |                         void (*__get_desc)(typeof(__next), char *) = get_desc; \

Signed-off-by: Maíra Canal <mairacanal@riseup.net>
---
 Documentation/dev-tools/kunit/usage.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
index d62a04255c2e..8e72fb277058 100644
--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -517,7 +517,7 @@ By reusing the same ``cases`` array from above, we can write the test as a
 	};
 
 	// Need a helper function to generate a name for each test case.
-	static void case_to_desc(const struct sha1_test_case *t, char *desc)
+	static void case_to_desc(struct sha1_test_case *t, char *desc)
 	{
 		strcpy(desc, t->str);
 	}
-- 
2.36.1

Re: [PATCH] Documentation: Kunit: Fix example with compilation error

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 2479 bytes --]

On Sat, Jul 2, 2022 at 2:17 AM Maíra Canal <mairacanal@riseup.net> wrote:
>
> The Parameterized Testing example contains a compilation error, as the
> signature for the description helper function should be void(*)(struct
> sha1_test_case *, char *), so the struct should not be const. This is
> warned by Clang:
>
> error: initialization of ‘void (*)(struct sha1_test_case *, char *)’
> from incompatible pointer type ‘void (*)(const struct sha1_test_case *,
> char *)’ [-Werror=incompatible-pointer-types]
>     33 | KUNIT_ARRAY_PARAM(sha1, cases, case_to_desc);
>        |                                ^~~~~~~~~~~~
> ../include/kunit/test.h:1339:70: note: in definition of macro
> ‘KUNIT_ARRAY_PARAM’
> 1339 |                         void (*__get_desc)(typeof(__next), char *) = get_desc; \
>
> Signed-off-by: Maíra Canal <mairacanal@riseup.net>
> ---

Thanks for catching this. The change to the documentation looks good,
but it may be better to change the array to be:
const struct cases[] = { ... }

That matches most of the existing uses of this, such as the mctp test,
and encourages the use of const in cases (like the example) where it
makes sense.

I'm okay with it either way, though: they're both valid.

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

>  Documentation/dev-tools/kunit/usage.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> index d62a04255c2e..8e72fb277058 100644
> --- a/Documentation/dev-tools/kunit/usage.rst
> +++ b/Documentation/dev-tools/kunit/usage.rst
> @@ -517,7 +517,7 @@ By reusing the same ``cases`` array from above, we can write the test as a
>         };
>
>         // Need a helper function to generate a name for each test case.
> -       static void case_to_desc(const struct sha1_test_case *t, char *desc)
> +       static void case_to_desc(struct sha1_test_case *t, char *desc)
>         {
>                 strcpy(desc, t->str);
>         }
> --
> 2.36.1
>
> --
> 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/20220701181723.349165-1-mairacanal%40riseup.net.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH] Documentation: Kunit: Fix example with compilation error

[Maíra Canal]

On 7/2/22 01:32, David Gow wrote:
> On Sat, Jul 2, 2022 at 2:17 AM Maíra Canal <mairacanal@riseup.net> wrote:
>>
>> The Parameterized Testing example contains a compilation error, as the
>> signature for the description helper function should be void(*)(struct
>> sha1_test_case *, char *), so the struct should not be const. This is
>> warned by Clang:
>>
>> error: initialization of ‘void (*)(struct sha1_test_case *, char *)’
>> from incompatible pointer type ‘void (*)(const struct sha1_test_case *,
>> char *)’ [-Werror=incompatible-pointer-types]
>>     33 | KUNIT_ARRAY_PARAM(sha1, cases, case_to_desc);
>>        |                                ^~~~~~~~~~~~
>> ../include/kunit/test.h:1339:70: note: in definition of macro
>> ‘KUNIT_ARRAY_PARAM’
>> 1339 |                         void (*__get_desc)(typeof(__next), char *) = get_desc; \
>>
>> Signed-off-by: Maíra Canal <mairacanal@riseup.net>
>> ---
> 
> Thanks for catching this. The change to the documentation looks good,
> but it may be better to change the array to be:
> const struct cases[] = { ... }

I missed that! Would you rather that I change it on a v2?

Best Regards,
- Maíra Canal

> 
> That matches most of the existing uses of this, such as the mctp test,
> and encourages the use of const in cases (like the example) where it
> makes sense.
> 
> I'm okay with it either way, though: they're both valid.
> 
> Reviewed-by: David Gow <davidgow@google.com>
> 
> Cheers,
> -- David
> 
>>  Documentation/dev-tools/kunit/usage.rst | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
>> index d62a04255c2e..8e72fb277058 100644
>> --- a/Documentation/dev-tools/kunit/usage.rst
>> +++ b/Documentation/dev-tools/kunit/usage.rst
>> @@ -517,7 +517,7 @@ By reusing the same ``cases`` array from above, we can write the test as a
>>         };
>>
>>         // Need a helper function to generate a name for each test case.
>> -       static void case_to_desc(const struct sha1_test_case *t, char *desc)
>> +       static void case_to_desc(struct sha1_test_case *t, char *desc)
>>         {
>>                 strcpy(desc, t->str);
>>         }
>> --
>> 2.36.1
>>
>> --

Re: [PATCH] Documentation: Kunit: Fix example with compilation error

[David Gow]

On Sat, Jul 2, 2022 at 9:35 PM Maíra Canal <mairacanal@riseup.net> wrote:
>
> On 7/2/22 01:32, David Gow wrote:
> > On Sat, Jul 2, 2022 at 2:17 AM Maíra Canal <mairacanal@riseup.net> wrote:
> >>
> >> The Parameterized Testing example contains a compilation error, as the
> >> signature for the description helper function should be void(*)(struct
> >> sha1_test_case *, char *), so the struct should not be const. This is
> >> warned by Clang:
> >>
> >> error: initialization of ‘void (*)(struct sha1_test_case *, char *)’
> >> from incompatible pointer type ‘void (*)(const struct sha1_test_case *,
> >> char *)’ [-Werror=incompatible-pointer-types]
> >>     33 | KUNIT_ARRAY_PARAM(sha1, cases, case_to_desc);
> >>        |                                ^~~~~~~~~~~~
> >> ../include/kunit/test.h:1339:70: note: in definition of macro
> >> ‘KUNIT_ARRAY_PARAM’
> >> 1339 |                         void (*__get_desc)(typeof(__next), char *) = get_desc; \
> >>
> >> Signed-off-by: Maíra Canal <mairacanal@riseup.net>
> >> ---
> >
> > Thanks for catching this. The change to the documentation looks good,
> > but it may be better to change the array to be:
> > const struct cases[] = { ... }
>
> I missed that! Would you rather that I change it on a v2?
>
> Best Regards,
> - Maíra Canal
>

Yeah, I think that'd be best.

No need to hurry, though: Brendan et al are away for the US long weekend.

Cheers,
-- David

> > That matches most of the existing uses of this, such as the mctp test,
> > and encourages the use of const in cases (like the example) where it
> > makes sense.
> >
> > I'm okay with it either way, though: they're both valid.
> >
> > Reviewed-by: David Gow <davidgow@google.com>
> >
> > Cheers,
> > -- David
> >
> >>  Documentation/dev-tools/kunit/usage.rst | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> >> index d62a04255c2e..8e72fb277058 100644
> >> --- a/Documentation/dev-tools/kunit/usage.rst
> >> +++ b/Documentation/dev-tools/kunit/usage.rst
> >> @@ -517,7 +517,7 @@ By reusing the same ``cases`` array from above, we can write the test as a
> >>         };
> >>
> >>         // Need a helper function to generate a name for each test case.
> >> -       static void case_to_desc(const struct sha1_test_case *t, char *desc)
> >> +       static void case_to_desc(struct sha1_test_case *t, char *desc)
> >>         {
> >>                 strcpy(desc, t->str);
> >>         }
> >> --
> >> 2.36.1
> >>
> >> --