| Message ID | 20220108012304.1049587-3-dlatypov@google.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: refactor assertions to use less stack | expand |
On Fri, Jan 7, 2022 at 8:23 PM 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_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. > > 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> Tiny nit, see below. Otherwise: Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > --- > include/kunit/test.h | 20 ++++++++++---------- > lib/kunit/test.c | 13 ++++--------- > 2 files changed, 14 insertions(+), 19 deletions(-) > > diff --git a/include/kunit/test.h b/include/kunit/test.h > index b26400731c02..690a28dfc795 100644 > --- a/include/kunit/test.h > +++ b/include/kunit/test.h > @@ -770,18 +770,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_failed_assertion(struct kunit *test, > + struct kunit_assert *assert, > + const char *fmt, ...); Tiny nit: I think this should be kunit_fail_assertion. I think functions should be in the active tense, imperative mood since when you call a function you are telling it to do something. Also, do we need to worry about this getting confused with KUNIT_FAIL, or KUNIT_FAIL_ASSERTION: https://elixir.bootlin.com/linux/v5.16/source/include/kunit/test.h#L788 ? > #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 (!(pass)) { \ > + struct assert_class __assertion = INITIALIZER; \ > + kunit_failed_assertion(test, \ > + &__assertion.assert, \ > + fmt, \ > + ##__VA_ARGS__); \ > + } \ > } while (0) > > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index c7ed4aabec04..5ad671745483 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_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_failed_assertion); > > void kunit_init_test(struct kunit *test, const char *name, char *log) > { > -- > 2.34.1.575.g55b058a8bb-goog >
On Mon, Jan 10, 2022 at 2:21 PM Brendan Higgins <brendanhiggins@google.com> wrote: > > On Fri, Jan 7, 2022 at 8:23 PM 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_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. > > > > 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> > > Tiny nit, see below. Otherwise: > > Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > > > --- > > include/kunit/test.h | 20 ++++++++++---------- > > lib/kunit/test.c | 13 ++++--------- > > 2 files changed, 14 insertions(+), 19 deletions(-) > > > > diff --git a/include/kunit/test.h b/include/kunit/test.h > > index b26400731c02..690a28dfc795 100644 > > --- a/include/kunit/test.h > > +++ b/include/kunit/test.h > > @@ -770,18 +770,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_failed_assertion(struct kunit *test, > > + struct kunit_assert *assert, > > + const char *fmt, ...); > > Tiny nit: I think this should be kunit_fail_assertion. I think > functions should be in the active tense, imperative mood since when > you call a function you are telling it to do something. > > Also, do we need to worry about this getting confused with KUNIT_FAIL, > or KUNIT_FAIL_ASSERTION: So do we want to try and pick a different name from kunit_fail_assertion() to avoid confusion with the macro? That's partly why I went with past tense. Perhaps: "kunit_do_assertion() => kunit_do_failed_assertion()" instead? Tangent: we have some similar confusing names, e.g. KUNIT_ASSERTION is both the name of a macro and an enum (kunit_assert_type), and those have the exact same case. > > https://elixir.bootlin.com/linux/v5.16/source/include/kunit/test.h#L788 > > ? > > > #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 (!(pass)) { \ > > + struct assert_class __assertion = INITIALIZER; \ > > + kunit_failed_assertion(test, \ > > + &__assertion.assert, \ > > + fmt, \ > > + ##__VA_ARGS__); \ > > + } \ > > } while (0) > > > > > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > > index c7ed4aabec04..5ad671745483 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_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_failed_assertion); > > > > void kunit_init_test(struct kunit *test, const char *name, char *log) > > { > > -- > > 2.34.1.575.g55b058a8bb-goog > >
On Tue, Jan 11, 2022 at 6:33 AM Daniel Latypov <dlatypov@google.com> wrote: > > On Mon, Jan 10, 2022 at 2:21 PM Brendan Higgins > <brendanhiggins@google.com> wrote: > > > > On Fri, Jan 7, 2022 at 8:23 PM 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_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. > > > > > > 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> > > > > Tiny nit, see below. Otherwise: > > > > Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > > > > > --- > > > include/kunit/test.h | 20 ++++++++++---------- > > > lib/kunit/test.c | 13 ++++--------- > > > 2 files changed, 14 insertions(+), 19 deletions(-) > > > > > > diff --git a/include/kunit/test.h b/include/kunit/test.h > > > index b26400731c02..690a28dfc795 100644 > > > --- a/include/kunit/test.h > > > +++ b/include/kunit/test.h > > > @@ -770,18 +770,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_failed_assertion(struct kunit *test, > > > + struct kunit_assert *assert, > > > + const char *fmt, ...); > > > > Tiny nit: I think this should be kunit_fail_assertion. I think > > functions should be in the active tense, imperative mood since when > > you call a function you are telling it to do something. > > > > Also, do we need to worry about this getting confused with KUNIT_FAIL, > > or KUNIT_FAIL_ASSERTION: > > So do we want to try and pick a different name from > kunit_fail_assertion() to avoid confusion with the macro? > That's partly why I went with past tense. > Perhaps: "kunit_do_assertion() => kunit_do_failed_assertion()" instead? I'm not particularly picky about the name personally. But if I had to join the bikeshedding, I'd probably go with kunit_assertion_fail() or similar (kunit_assertion_failed works too, past-tense-wise.) But kunit_do_fail{,ed}_assertion() would work too. > > Tangent: we have some similar confusing names, e.g. KUNIT_ASSERTION is > both the name of a macro and an enum (kunit_assert_type), and those > have the exact same case. > > > > > https://elixir.bootlin.com/linux/v5.16/source/include/kunit/test.h#L788 > > > > ? > > > > > #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 (!(pass)) { \ > > > + struct assert_class __assertion = INITIALIZER; \ > > > + kunit_failed_assertion(test, \ > > > + &__assertion.assert, \ > > > + fmt, \ > > > + ##__VA_ARGS__); \ > > > + } \ > > > } while (0) > > > > > > > > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > > > index c7ed4aabec04..5ad671745483 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_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_failed_assertion); > > > > > > void kunit_init_test(struct kunit *test, const char *name, char *log) > > > { > > > -- > > > 2.34.1.575.g55b058a8bb-goog > > >
On Mon, Jan 10, 2022 at 10:51 PM David Gow <davidgow@google.com> wrote: > > On Tue, Jan 11, 2022 at 6:33 AM Daniel Latypov <dlatypov@google.com> wrote: > > > > On Mon, Jan 10, 2022 at 2:21 PM Brendan Higgins > > <brendanhiggins@google.com> wrote: > > > > > > On Fri, Jan 7, 2022 at 8:23 PM 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_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. > > > > > > > > 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> > > > > > > Tiny nit, see below. Otherwise: > > > > > > Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > > > > > > > --- > > > > include/kunit/test.h | 20 ++++++++++---------- > > > > lib/kunit/test.c | 13 ++++--------- > > > > 2 files changed, 14 insertions(+), 19 deletions(-) > > > > > > > > diff --git a/include/kunit/test.h b/include/kunit/test.h > > > > index b26400731c02..690a28dfc795 100644 > > > > --- a/include/kunit/test.h > > > > +++ b/include/kunit/test.h > > > > @@ -770,18 +770,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_failed_assertion(struct kunit *test, > > > > + struct kunit_assert *assert, > > > > + const char *fmt, ...); > > > > > > Tiny nit: I think this should be kunit_fail_assertion. I think > > > functions should be in the active tense, imperative mood since when > > > you call a function you are telling it to do something. > > > > > > Also, do we need to worry about this getting confused with KUNIT_FAIL, > > > or KUNIT_FAIL_ASSERTION: > > > > So do we want to try and pick a different name from > > kunit_fail_assertion() to avoid confusion with the macro? > > That's partly why I went with past tense. > > Perhaps: "kunit_do_assertion() => kunit_do_failed_assertion()" instead? > > I'm not particularly picky about the name personally. But if I had to > join the bikeshedding, I'd probably go with kunit_assertion_fail() or > similar (kunit_assertion_failed works too, past-tense-wise.) > > But kunit_do_fail{,ed}_assertion() would work too. I've gone ahead and locally renamed it to kunit_do_failed_assertion(). Talking offline, Brendan seemed ok with it, so we have 2 votes of "it's good enough". > > > > > > Tangent: we have some similar confusing names, e.g. KUNIT_ASSERTION is > > both the name of a macro and an enum (kunit_assert_type), and those > > have the exact same case. > > > > > > > > https://elixir.bootlin.com/linux/v5.16/source/include/kunit/test.h#L788 > > > > > > ? > > > > > > > #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 (!(pass)) { \ > > > > + struct assert_class __assertion = INITIALIZER; \ > > > > + kunit_failed_assertion(test, \ > > > > + &__assertion.assert, \ > > > > + fmt, \ > > > > + ##__VA_ARGS__); \ > > > > + } \ > > > > } while (0) > > > > > > > > > > > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > > > > index c7ed4aabec04..5ad671745483 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_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_failed_assertion); > > > > > > > > void kunit_init_test(struct kunit *test, const char *name, char *log) > > > > { > > > > -- > > > > 2.34.1.575.g55b058a8bb-goog > > > >
diff --git a/include/kunit/test.h b/include/kunit/test.h index b26400731c02..690a28dfc795 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -770,18 +770,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_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 (!(pass)) { \ + struct assert_class __assertion = INITIALIZER; \ + kunit_failed_assertion(test, \ + &__assertion.assert, \ + fmt, \ + ##__VA_ARGS__); \ + } \ } while (0) diff --git a/lib/kunit/test.c b/lib/kunit/test.c index c7ed4aabec04..5ad671745483 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_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_failed_assertion); void kunit_init_test(struct kunit *test, const char *name, char *log) {
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_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. 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> --- include/kunit/test.h | 20 ++++++++++---------- lib/kunit/test.c | 13 ++++--------- 2 files changed, 14 insertions(+), 19 deletions(-)