| Message ID | 20220127215222.159049-1-dlatypov@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c2741453478badf571ef020d160053e8d5e1ba94 |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: cleanup assertion macro internal variables | expand |
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 >
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>
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>
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, \
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(-)