| Message ID | 20220108012304.1049587-5-dlatypov@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | dd640d70874bd27fb081d444252677766321c32f |
| 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: > > We call this function first thing for all the assertion `format()` > functions. > This is the part that prints the file and line number and assertion type > (EXPECTATION, ASSERTION). > > Having it as part of the format functions lets us have the flexibility > to not print that information (or print it differently) for new > assertion types, but I think this we don't need that. nit: drop the "this". > And in the future, we'd like to consider factoring that data (file, > line#, type) out of the kunit_assert struct and into a `static` > variable, as Linus suggested [1], so we'd need to extract it anyways. > > [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > --- > lib/kunit/assert.c | 6 ------ > lib/kunit/test.c | 1 + > 2 files changed, 1 insertion(+), 6 deletions(-) > > diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c > index b972bda61c0c..4d9a1295efc7 100644 > --- a/lib/kunit/assert.c > +++ b/lib/kunit/assert.c > @@ -40,7 +40,6 @@ EXPORT_SYMBOL_GPL(kunit_assert_print_msg); > void kunit_fail_assert_format(const struct kunit_assert *assert, > struct string_stream *stream) > { > - kunit_base_assert_format(assert, stream); > string_stream_add(stream, "%pV", &assert->message); > } > EXPORT_SYMBOL_GPL(kunit_fail_assert_format); > @@ -52,7 +51,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert, > > unary_assert = container_of(assert, struct kunit_unary_assert, assert); > > - kunit_base_assert_format(assert, stream); > if (unary_assert->expected_true) > string_stream_add(stream, > KUNIT_SUBTEST_INDENT "Expected %s to be true, but is false\n", > @@ -73,7 +71,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, > ptr_assert = container_of(assert, struct kunit_ptr_not_err_assert, > assert); > > - kunit_base_assert_format(assert, stream); > if (!ptr_assert->value) { > string_stream_add(stream, > KUNIT_SUBTEST_INDENT "Expected %s is not null, but is\n", > @@ -119,7 +116,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert, > binary_assert = container_of(assert, struct kunit_binary_assert, > assert); > > - kunit_base_assert_format(assert, stream); > string_stream_add(stream, > KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n", > binary_assert->left_text, > @@ -147,7 +143,6 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert, > binary_assert = container_of(assert, struct kunit_binary_ptr_assert, > assert); > > - kunit_base_assert_format(assert, stream); > string_stream_add(stream, > KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n", > binary_assert->left_text, > @@ -187,7 +182,6 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert, > binary_assert = container_of(assert, struct kunit_binary_str_assert, > assert); > > - kunit_base_assert_format(assert, stream); > string_stream_add(stream, > KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n", > binary_assert->left_text, > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index 5ad671745483..735c1b67d843 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -255,6 +255,7 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert) > return; > } > > + kunit_base_assert_format(assert, stream); I think my thinking in having this function called by the other assert functions was to take advantage of inheritance. I was treating kunit_base_assert_format as the parent method that other methods were inheriting from, so I wanted to have them inherit some of the common behavior by calling the original function. If you decide to make this change, I think it would be a good idea to change the name of kunit_base_assert_format to not mislead to this effect. > assert->format(assert, stream); > > kunit_print_string_stream(test, stream); > -- > 2.34.1.575.g55b058a8bb-goog >
On Mon, Jan 10, 2022 at 2:32 PM 'Brendan Higgins' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > On Fri, Jan 7, 2022 at 8:23 PM Daniel Latypov <dlatypov@google.com> wrote: > > > > We call this function first thing for all the assertion `format()` > > functions. > > This is the part that prints the file and line number and assertion type > > (EXPECTATION, ASSERTION). > > > > Having it as part of the format functions lets us have the flexibility > > to not print that information (or print it differently) for new > > assertion types, but I think this we don't need that. > > nit: drop the "this". > > > And in the future, we'd like to consider factoring that data (file, > > line#, type) out of the kunit_assert struct and into a `static` > > variable, as Linus suggested [1], so we'd need to extract it anyways. > > > > [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ > > > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > --- > > lib/kunit/assert.c | 6 ------ > > lib/kunit/test.c | 1 + > > 2 files changed, 1 insertion(+), 6 deletions(-) > > > > diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c > > index b972bda61c0c..4d9a1295efc7 100644 > > --- a/lib/kunit/assert.c > > +++ b/lib/kunit/assert.c > > @@ -40,7 +40,6 @@ EXPORT_SYMBOL_GPL(kunit_assert_print_msg); > > void kunit_fail_assert_format(const struct kunit_assert *assert, > > struct string_stream *stream) > > { > > - kunit_base_assert_format(assert, stream); > > string_stream_add(stream, "%pV", &assert->message); > > } > > EXPORT_SYMBOL_GPL(kunit_fail_assert_format); > > @@ -52,7 +51,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert, > > > > unary_assert = container_of(assert, struct kunit_unary_assert, assert); > > > > - kunit_base_assert_format(assert, stream); > > if (unary_assert->expected_true) > > string_stream_add(stream, > > KUNIT_SUBTEST_INDENT "Expected %s to be true, but is false\n", > > @@ -73,7 +71,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, > > ptr_assert = container_of(assert, struct kunit_ptr_not_err_assert, > > assert); > > > > - kunit_base_assert_format(assert, stream); > > if (!ptr_assert->value) { > > string_stream_add(stream, > > KUNIT_SUBTEST_INDENT "Expected %s is not null, but is\n", > > @@ -119,7 +116,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert, > > binary_assert = container_of(assert, struct kunit_binary_assert, > > assert); > > > > - kunit_base_assert_format(assert, stream); > > string_stream_add(stream, > > KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n", > > binary_assert->left_text, > > @@ -147,7 +143,6 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert, > > binary_assert = container_of(assert, struct kunit_binary_ptr_assert, > > assert); > > > > - kunit_base_assert_format(assert, stream); > > string_stream_add(stream, > > KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n", > > binary_assert->left_text, > > @@ -187,7 +182,6 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert, > > binary_assert = container_of(assert, struct kunit_binary_str_assert, > > assert); > > > > - kunit_base_assert_format(assert, stream); > > string_stream_add(stream, > > KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n", > > binary_assert->left_text, > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > > index 5ad671745483..735c1b67d843 100644 > > --- a/lib/kunit/test.c > > +++ b/lib/kunit/test.c > > @@ -255,6 +255,7 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert) > > return; > > } > > > > + kunit_base_assert_format(assert, stream); > > I think my thinking in having this function called by the other assert > functions was to take advantage of inheritance. I was treating > kunit_base_assert_format as the parent method that other methods were > inheriting from, so I wanted to have them inherit some of the common > behavior by calling the original function. > > If you decide to make this change, I think it would be a good idea to > change the name of kunit_base_assert_format to not mislead to this > effect. The child patch renames it to kunit_assert_prologue(). I can rename it in this change if you prefer. I had just initially left it with the same name to keep this diff a bit smaller and more focused. But now you point it out, I think it would be cleaner to rename it here. > > > assert->format(assert, stream); > > > > kunit_print_string_stream(test, stream); > > -- > > 2.34.1.575.g55b058a8bb-goog > > > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/CAFd5g47r8aQBWPtt6ffHokqqN2sMi10p1Q5QA3xGVLTVDQh98Q%40mail.gmail.com.
On Sat, Jan 8, 2022 at 9:23 AM Daniel Latypov <dlatypov@google.com> wrote: > > We call this function first thing for all the assertion `format()` > functions. > This is the part that prints the file and line number and assertion type > (EXPECTATION, ASSERTION). > > Having it as part of the format functions lets us have the flexibility > to not print that information (or print it differently) for new > assertion types, but I think this we don't need that. > > And in the future, we'd like to consider factoring that data (file, > line#, type) out of the kunit_assert struct and into a `static` > variable, as Linus suggested [1], so we'd need to extract it anyways. > > [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > --- Looks good to me, thanks! Reviewed-by: David Gow <davidgow@google.com> > lib/kunit/assert.c | 6 ------ > lib/kunit/test.c | 1 + > 2 files changed, 1 insertion(+), 6 deletions(-) > > diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c > index b972bda61c0c..4d9a1295efc7 100644 > --- a/lib/kunit/assert.c > +++ b/lib/kunit/assert.c > @@ -40,7 +40,6 @@ EXPORT_SYMBOL_GPL(kunit_assert_print_msg); > void kunit_fail_assert_format(const struct kunit_assert *assert, > struct string_stream *stream) > { > - kunit_base_assert_format(assert, stream); > string_stream_add(stream, "%pV", &assert->message); > } > EXPORT_SYMBOL_GPL(kunit_fail_assert_format); > @@ -52,7 +51,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert, > > unary_assert = container_of(assert, struct kunit_unary_assert, assert); > > - kunit_base_assert_format(assert, stream); > if (unary_assert->expected_true) > string_stream_add(stream, > KUNIT_SUBTEST_INDENT "Expected %s to be true, but is false\n", > @@ -73,7 +71,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, > ptr_assert = container_of(assert, struct kunit_ptr_not_err_assert, > assert); > > - kunit_base_assert_format(assert, stream); > if (!ptr_assert->value) { > string_stream_add(stream, > KUNIT_SUBTEST_INDENT "Expected %s is not null, but is\n", > @@ -119,7 +116,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert, > binary_assert = container_of(assert, struct kunit_binary_assert, > assert); > > - kunit_base_assert_format(assert, stream); > string_stream_add(stream, > KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n", > binary_assert->left_text, > @@ -147,7 +143,6 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert, > binary_assert = container_of(assert, struct kunit_binary_ptr_assert, > assert); > > - kunit_base_assert_format(assert, stream); > string_stream_add(stream, > KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n", > binary_assert->left_text, > @@ -187,7 +182,6 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert, > binary_assert = container_of(assert, struct kunit_binary_str_assert, > assert); > > - kunit_base_assert_format(assert, stream); > string_stream_add(stream, > KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n", > binary_assert->left_text, > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index 5ad671745483..735c1b67d843 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -255,6 +255,7 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert) > return; > } > > + kunit_base_assert_format(assert, stream); > assert->format(assert, stream); > > kunit_print_string_stream(test, stream); > -- > 2.34.1.575.g55b058a8bb-goog >
diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c index b972bda61c0c..4d9a1295efc7 100644 --- a/lib/kunit/assert.c +++ b/lib/kunit/assert.c @@ -40,7 +40,6 @@ EXPORT_SYMBOL_GPL(kunit_assert_print_msg); void kunit_fail_assert_format(const struct kunit_assert *assert, struct string_stream *stream) { - kunit_base_assert_format(assert, stream); string_stream_add(stream, "%pV", &assert->message); } EXPORT_SYMBOL_GPL(kunit_fail_assert_format); @@ -52,7 +51,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert, unary_assert = container_of(assert, struct kunit_unary_assert, assert); - kunit_base_assert_format(assert, stream); if (unary_assert->expected_true) string_stream_add(stream, KUNIT_SUBTEST_INDENT "Expected %s to be true, but is false\n", @@ -73,7 +71,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, ptr_assert = container_of(assert, struct kunit_ptr_not_err_assert, assert); - kunit_base_assert_format(assert, stream); if (!ptr_assert->value) { string_stream_add(stream, KUNIT_SUBTEST_INDENT "Expected %s is not null, but is\n", @@ -119,7 +116,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert, binary_assert = container_of(assert, struct kunit_binary_assert, assert); - kunit_base_assert_format(assert, stream); string_stream_add(stream, KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n", binary_assert->left_text, @@ -147,7 +143,6 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert, binary_assert = container_of(assert, struct kunit_binary_ptr_assert, assert); - kunit_base_assert_format(assert, stream); string_stream_add(stream, KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n", binary_assert->left_text, @@ -187,7 +182,6 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert, binary_assert = container_of(assert, struct kunit_binary_str_assert, assert); - kunit_base_assert_format(assert, stream); string_stream_add(stream, KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n", binary_assert->left_text, diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 5ad671745483..735c1b67d843 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -255,6 +255,7 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert) return; } + kunit_base_assert_format(assert, stream); assert->format(assert, stream); kunit_print_string_stream(test, stream);
We call this function first thing for all the assertion `format()` functions. This is the part that prints the file and line number and assertion type (EXPECTATION, ASSERTION). Having it as part of the format functions lets us have the flexibility to not print that information (or print it differently) for new assertion types, but I think this we don't need that. And in the future, we'd like to consider factoring that data (file, line#, type) out of the kunit_assert struct and into a `static` variable, as Linus suggested [1], so we'd need to extract it anyways. [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ Signed-off-by: Daniel Latypov <dlatypov@google.com> --- lib/kunit/assert.c | 6 ------ lib/kunit/test.c | 1 + 2 files changed, 1 insertion(+), 6 deletions(-)