[PATCH] kunit: cleanup assertion macro internal variables

[PATCH] kunit: cleanup assertion macro internal variables

[Daniel Latypov]

All the operands should be tagged `const`.
We're only assigning them to variables so that we can compare them (e.g.
check if left == right, etc.) and avoid evaluating expressions multiple
times.

There's no need for them to be mutable.

Also rename the helper variable `loc` to `__loc` like we do with
`__assertion` and `__strs` to avoid potential name collisions with user
code.

Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
Note: this patch is based on top of
https://lore.kernel.org/all/20220125210011.3817742-4-dlatypov@google.com/
There is no semantic dependency between the patches, but they touch
adjacent lines.
---
 include/kunit/test.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 088ff394ae94..00b9ff7783ab 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -779,10 +779,10 @@ void kunit_do_failed_assertion(struct kunit *test,
 
 #define KUNIT_ASSERTION(test, assert_type, pass, assert_class, INITIALIZER, fmt, ...) do { \
 	if (unlikely(!(pass))) {					       \
-		static const struct kunit_loc loc = KUNIT_CURRENT_LOC;	       \
+		static const struct kunit_loc __loc = KUNIT_CURRENT_LOC;       \
 		struct assert_class __assertion = INITIALIZER;		       \
 		kunit_do_failed_assertion(test,				       \
-					  &loc,				       \
+					  &__loc,			       \
 					  assert_type,			       \
 					  &__assertion.assert,		       \
 					  fmt,				       \
@@ -872,8 +872,8 @@ void kunit_do_failed_assertion(struct kunit *test,
 				    fmt,				       \
 				    ...)				       \
 do {									       \
-	typeof(left) __left = (left);					       \
-	typeof(right) __right = (right);				       \
+	const typeof(left) __left = (left);				       \
+	const typeof(right) __right = (right);				       \
 	static const struct kunit_binary_assert_text __text = {		       \
 		.operation = #op,					       \
 		.left_text = #left,					       \
@@ -956,7 +956,7 @@ do {									       \
 						fmt,			       \
 						...)			       \
 do {									       \
-	typeof(ptr) __ptr = (ptr);					       \
+	const typeof(ptr) __ptr = (ptr);				       \
 									       \
 	KUNIT_ASSERTION(test,						       \
 			assert_type,					       \
-- 
2.35.0.rc2.247.g8bbb082509-goog

Re: [PATCH] kunit: cleanup assertion macro internal variables

[David Gow]

On Fri, Jan 28, 2022 at 5:52 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> All the operands should be tagged `const`.
> We're only assigning them to variables so that we can compare them (e.g.
> check if left == right, etc.) and avoid evaluating expressions multiple
> times.
>
> There's no need for them to be mutable.
>
> Also rename the helper variable `loc` to `__loc` like we do with
> `__assertion` and `__strs` to avoid potential name collisions with user
> code.
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---
> Note: this patch is based on top of
> https://lore.kernel.org/all/20220125210011.3817742-4-dlatypov@google.com/
> There is no semantic dependency between the patches, but they touch
> adjacent lines.
> ---

Looks good.

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

Cheers,
-- David

>  include/kunit/test.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 088ff394ae94..00b9ff7783ab 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -779,10 +779,10 @@ void kunit_do_failed_assertion(struct kunit *test,
>
>  #define KUNIT_ASSERTION(test, assert_type, pass, assert_class, INITIALIZER, fmt, ...) do { \
>         if (unlikely(!(pass))) {                                               \
> -               static const struct kunit_loc loc = KUNIT_CURRENT_LOC;         \
> +               static const struct kunit_loc __loc = KUNIT_CURRENT_LOC;       \
>                 struct assert_class __assertion = INITIALIZER;                 \
>                 kunit_do_failed_assertion(test,                                \
> -                                         &loc,                                \
> +                                         &__loc,                              \
>                                           assert_type,                         \
>                                           &__assertion.assert,                 \
>                                           fmt,                                 \
> @@ -872,8 +872,8 @@ void kunit_do_failed_assertion(struct kunit *test,
>                                     fmt,                                       \
>                                     ...)                                       \
>  do {                                                                          \
> -       typeof(left) __left = (left);                                          \
> -       typeof(right) __right = (right);                                       \
> +       const typeof(left) __left = (left);                                    \
> +       const typeof(right) __right = (right);                                 \
>         static const struct kunit_binary_assert_text __text = {                \
>                 .operation = #op,                                              \
>                 .left_text = #left,                                            \
> @@ -956,7 +956,7 @@ do {                                                                               \
>                                                 fmt,                           \
>                                                 ...)                           \
>  do {                                                                          \
> -       typeof(ptr) __ptr = (ptr);                                             \
> +       const typeof(ptr) __ptr = (ptr);                                       \
>                                                                                \
>         KUNIT_ASSERTION(test,                                                  \
>                         assert_type,                                           \
> --
> 2.35.0.rc2.247.g8bbb082509-goog
>

Re: [PATCH] kunit: cleanup assertion macro internal variables

[Brendan Higgins]

On Thu, Jan 27, 2022 at 4:52 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> All the operands should be tagged `const`.
> We're only assigning them to variables so that we can compare them (e.g.
> check if left == right, etc.) and avoid evaluating expressions multiple
> times.
>
> There's no need for them to be mutable.

Agreed.

> Also rename the helper variable `loc` to `__loc` like we do with
> `__assertion` and `__strs` to avoid potential name collisions with user
> code.

Probably not necessary since we create a new code block (we are inside
of an if-statement, do-while-loop, etc), but I don't really care
either way.

> Signed-off-by: Daniel Latypov <dlatypov@google.com>

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

Re: [PATCH] kunit: cleanup assertion macro internal variables

[Daniel Latypov]

On Fri, Jan 28, 2022 at 1:21 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Thu, Jan 27, 2022 at 4:52 PM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > All the operands should be tagged `const`.
> > We're only assigning them to variables so that we can compare them (e.g.
> > check if left == right, etc.) and avoid evaluating expressions multiple
> > times.
> >
> > There's no need for them to be mutable.
>
> Agreed.
>
> > Also rename the helper variable `loc` to `__loc` like we do with
> > `__assertion` and `__strs` to avoid potential name collisions with user
> > code.
>
> Probably not necessary since we create a new code block (we are inside
> of an if-statement, do-while-loop, etc), but I don't really care
> either way.

You're totally right that this doesn't matter with our current macros.

given
int loc = 42;
KUNIT_EXPECT_TRUE(test, loc);
KUNIT_EXPECT_EQ(test, loc, 42);

becomes
do {
        if (__builtin_expect(!!(!(!!(loc) == !!true)), 0)) {
                /* we don't use the operands in here, so `loc` is fine */
                static const struct kunit_loc loc = {
                        .file = "lib/kunit/kunit-example-test.c", .line = 25
                };
...
do {
        typeof(loc) __left = (loc);
        typeof(42) __right = (42);
        do {
                /* We never reference the expression again, so `loc` is fine */
                if (__builtin_expect(!!(!(__left == __right)), 0)) {
                        static const struct kunit_loc loc = {
                                .file = "lib/kunit/kunit-example-test.c",
                                .line = 24
                        };

But reminder: this was *not* the case until very recently.
Sau we didn't have my earlier patch to move the `if(!(passed))` check
into the macro.
Then we'd have issues, e.g.
../lib/kunit/kunit-example-test.c: In function ‘example_simple_test’:
../include/kunit/test.h:828:26: error: wrong type argument to unary
exclamation mark
  828 |                         !!(condition) == !!expected_true,
                \
      |
...
../lib/kunit/kunit-example-test.c:25:9: note: in expansion of macro
‘KUNIT_EXPECT_TRUE’
   25 |         KUNIT_EXPECT_TRUE(test, loc);
      |         ^~~~~~~~~~~~~~~~~

So being defensive here lets us change up our implementation more freely.

>
> > Signed-off-by: Daniel Latypov <dlatypov@google.com>
>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>