diff mbox series

[v2,2/6] kunit: move check if assertion passed into the macros

Message ID 20220111194231.1797841-3-dlatypov@google.com (mailing list archive)
State Accepted
Commit 4fdacef8ac5a5382eeb1bc6fc2632d71a09d52cd
Delegated to: Brendan Higgins
Headers show
Series kunit: refactor assertions to use less stack | expand

Commit Message

Daniel Latypov Jan. 11, 2022, 7:42 p.m. UTC
Currently the code always calls kunit_do_assertion() even though it does
nothing when `pass` is true.

This change moves the `if(!(pass))` check into the macro instead
and renames the function to kunit_do_failed_assertion().
I feel this a  bit easier to read and understand.

This has the potential upside of avoiding a function call that does
nothing most of the time (assuming your tests are passing) but comes
with the downside of generating a bit more code and branches. We try to
mitigate the branches by tagging them with `unlikely()`.

This also means we don't have to initialize structs that we don't need,
which will become a tiny bit more expensive if we switch over to using
static variables to try and reduce stack usage. (There's runtime code
to check if the variable has been initialized yet or not).

Signed-off-by: Daniel Latypov <dlatypov@google.com>
Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
---
 include/kunit/test.h | 21 +++++++++++----------
 lib/kunit/test.c     | 13 ++++---------
 2 files changed, 15 insertions(+), 19 deletions(-)

Comments

David Gow Jan. 12, 2022, 1:27 a.m. UTC | #1
On Wed, Jan 12, 2022 at 3:42 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> Currently the code always calls kunit_do_assertion() even though it does
> nothing when `pass` is true.
>
> This change moves the `if(!(pass))` check into the macro instead
> and renames the function to kunit_do_failed_assertion().
> I feel this a  bit easier to read and understand.
>
> This has the potential upside of avoiding a function call that does
> nothing most of the time (assuming your tests are passing) but comes
> with the downside of generating a bit more code and branches. We try to
> mitigate the branches by tagging them with `unlikely()`.
>
> This also means we don't have to initialize structs that we don't need,
> which will become a tiny bit more expensive if we switch over to using
> static variables to try and reduce stack usage. (There's runtime code
> to check if the variable has been initialized yet or not).
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> ---

This looks good. I'm still not 100% sold that putting the if() outside
the function is significantly easier to read, but I don't think it's
harder to read either, and getting rid of the function call is
probably worth it.

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

-- David

>  include/kunit/test.h | 21 +++++++++++----------
>  lib/kunit/test.c     | 13 ++++---------
>  2 files changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index b26400731c02..12cabd15449a 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -12,6 +12,7 @@
>  #include <kunit/assert.h>
>  #include <kunit/try-catch.h>
>
> +#include <linux/compiler.h>
>  #include <linux/container_of.h>
>  #include <linux/err.h>
>  #include <linux/init.h>
> @@ -770,18 +771,18 @@ void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...);
>   */
>  #define KUNIT_SUCCEED(test) do {} while (0)
>
> -void kunit_do_assertion(struct kunit *test,
> -                       struct kunit_assert *assert,
> -                       bool pass,
> -                       const char *fmt, ...);
> +void kunit_do_failed_assertion(struct kunit *test,
> +                              struct kunit_assert *assert,
> +                              const char *fmt, ...);
>
>  #define KUNIT_ASSERTION(test, pass, assert_class, INITIALIZER, fmt, ...) do {  \
> -       struct assert_class __assertion = INITIALIZER;                         \
> -       kunit_do_assertion(test,                                               \
> -                          &__assertion.assert,                                \
> -                          pass,                                               \
> -                          fmt,                                                \
> -                          ##__VA_ARGS__);                                     \
> +       if (unlikely(!(pass))) {                                               \
> +               struct assert_class __assertion = INITIALIZER;                 \
> +               kunit_do_failed_assertion(test,                                \
> +                                         &__assertion.assert,                 \
> +                                         fmt,                                 \
> +                                         ##__VA_ARGS__);                      \
> +       }                                                                      \
>  } while (0)
>
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index c7ed4aabec04..3a52c321c280 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -275,16 +275,11 @@ static void __noreturn kunit_abort(struct kunit *test)
>         WARN_ONCE(true, "Throw could not abort from test!\n");
>  }
>
> -void kunit_do_assertion(struct kunit *test,
> -                       struct kunit_assert *assert,
> -                       bool pass,
> -                       const char *fmt, ...)
> +void kunit_do_failed_assertion(struct kunit *test,
> +                              struct kunit_assert *assert,
> +                              const char *fmt, ...)
>  {
>         va_list args;
> -
> -       if (pass)
> -               return;
> -
>         va_start(args, fmt);
>
>         assert->message.fmt = fmt;
> @@ -297,7 +292,7 @@ void kunit_do_assertion(struct kunit *test,
>         if (assert->type == KUNIT_ASSERTION)
>                 kunit_abort(test);
>  }
> -EXPORT_SYMBOL_GPL(kunit_do_assertion);
> +EXPORT_SYMBOL_GPL(kunit_do_failed_assertion);
>
>  void kunit_init_test(struct kunit *test, const char *name, char *log)
>  {
> --
> 2.34.1.575.g55b058a8bb-goog
>
diff mbox series

Patch

diff --git a/include/kunit/test.h b/include/kunit/test.h
index b26400731c02..12cabd15449a 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -12,6 +12,7 @@ 
 #include <kunit/assert.h>
 #include <kunit/try-catch.h>
 
+#include <linux/compiler.h>
 #include <linux/container_of.h>
 #include <linux/err.h>
 #include <linux/init.h>
@@ -770,18 +771,18 @@  void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...);
  */
 #define KUNIT_SUCCEED(test) do {} while (0)
 
-void kunit_do_assertion(struct kunit *test,
-			struct kunit_assert *assert,
-			bool pass,
-			const char *fmt, ...);
+void kunit_do_failed_assertion(struct kunit *test,
+			       struct kunit_assert *assert,
+			       const char *fmt, ...);
 
 #define KUNIT_ASSERTION(test, pass, assert_class, INITIALIZER, fmt, ...) do {  \
-	struct assert_class __assertion = INITIALIZER;			       \
-	kunit_do_assertion(test,					       \
-			   &__assertion.assert,				       \
-			   pass,					       \
-			   fmt,						       \
-			   ##__VA_ARGS__);				       \
+	if (unlikely(!(pass))) {					       \
+		struct assert_class __assertion = INITIALIZER;		       \
+		kunit_do_failed_assertion(test,				       \
+					  &__assertion.assert,		       \
+					  fmt,				       \
+					  ##__VA_ARGS__);		       \
+	}								       \
 } while (0)
 
 
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index c7ed4aabec04..3a52c321c280 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -275,16 +275,11 @@  static void __noreturn kunit_abort(struct kunit *test)
 	WARN_ONCE(true, "Throw could not abort from test!\n");
 }
 
-void kunit_do_assertion(struct kunit *test,
-			struct kunit_assert *assert,
-			bool pass,
-			const char *fmt, ...)
+void kunit_do_failed_assertion(struct kunit *test,
+			       struct kunit_assert *assert,
+			       const char *fmt, ...)
 {
 	va_list args;
-
-	if (pass)
-		return;
-
 	va_start(args, fmt);
 
 	assert->message.fmt = fmt;
@@ -297,7 +292,7 @@  void kunit_do_assertion(struct kunit *test,
 	if (assert->type == KUNIT_ASSERTION)
 		kunit_abort(test);
 }
-EXPORT_SYMBOL_GPL(kunit_do_assertion);
+EXPORT_SYMBOL_GPL(kunit_do_failed_assertion);
 
 void kunit_init_test(struct kunit *test, const char *name, char *log)
 {