2022-01-28 20:12:48

by Daniel Latypov

[permalink] [raw]
Subject: [PATCH] kunit: cleanup assertion macro internal variables

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 <[email protected]>
---
Note: this patch is based on top of
https://lore.kernel.org/all/[email protected]/
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


2022-01-29 16:25:36

by David Gow

[permalink] [raw]
Subject: Re: [PATCH] kunit: cleanup assertion macro internal variables

On Fri, Jan 28, 2022 at 5:52 AM Daniel Latypov <[email protected]> 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 <[email protected]>
> ---
> Note: this patch is based on top of
> https://lore.kernel.org/all/[email protected]/
> There is no semantic dependency between the patches, but they touch
> adjacent lines.
> ---

Looks good.

Reviewed-by: David Gow <[email protected]>

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
>

2022-01-31 11:40:28

by Brendan Higgins

[permalink] [raw]
Subject: Re: [PATCH] kunit: cleanup assertion macro internal variables

On Thu, Jan 27, 2022 at 4:52 PM Daniel Latypov <[email protected]> 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 <[email protected]>

Reviewed-by: Brendan Higgins <[email protected]>

2022-01-31 11:41:54

by Daniel Latypov

[permalink] [raw]
Subject: Re: [PATCH] kunit: cleanup assertion macro internal variables

On Fri, Jan 28, 2022 at 1:21 PM Brendan Higgins
<[email protected]> wrote:
>
> On Thu, Jan 27, 2022 at 4:52 PM Daniel Latypov <[email protected]> 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 <[email protected]>
>
> Reviewed-by: Brendan Higgins <[email protected]>