On Fri, Jan 7, 2022 at 8:23 PM Daniel Latypov <dlatypov@xxxxxxxxxx> 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@xxxxxxxxxx> > --- > 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 >
