RFC: https://lore.kernel.org/linux-kselftest/20220525154442.1438081-1-dlatypov@go... Changes since then: tweak commit messages, reformatting to make checkpatch.pl happy. Nothing substantial. Why send this out again now: the initial Rust patchset no longer contains the Kunit changes, so hopefully both series can go into 6.1 and later we can coordinate the update the kunit.rs wrapper.
This is a follow up to these three series: https://lore.kernel.org/all/20220113165931.451305-1-dlatypov@google.com/ https://lore.kernel.org/all/20220118223506.1701553-1-dlatypov@google.com/ https://lore.kernel.org/all/20220125210011.3817742-1-dlatypov@google.com/
The two goals of those series were a) reduce the size of struct kunit_assert and friends. (struct kunit_assert went from 48 => 8 bytes on UML.) b) simplify the internal code, mostly by deleting macros
This series goes further a) sizeof(struct kunit_assert) = 0 now b) e.g. we delete another class of macros (KUNIT_INIT_*_ASSERT_STRUCT)
Note: this does change the function signature of kunit_do_failed_assertion, so we'd need to update the rust wrapper in https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/kunit.rs, but hopefully it's just a simple change, e.g. maybe just like: @@ -38,9 +38,7 @@ }); static CONDITION: &'static $crate::str::CStr = $crate::c_str!(stringify!($cond)); static ASSERTION: UnaryAssert = UnaryAssert($crate::bindings::kunit_unary_assert { - assert: $crate::bindings::kunit_assert { - format: Some($crate::bindings::kunit_unary_assert_format), - }, + assert: $crate::bindings::kunit_assert {}, condition: CONDITION.as_char_ptr(), expected_true: true, }); @@ -67,6 +65,7 @@ core::ptr::addr_of!(LOCATION.0), $crate::bindings::kunit_assert_type_KUNIT_ASSERTION, core::ptr::addr_of!(ASSERTION.0.assert), + Some($crate::bindings::kunit_unary_assert_format), core::ptr::null(), ); }
Daniel Latypov (4): kunit: remove format func from struct kunit_assert, get it to 0 bytes kunit: rename base KUNIT_ASSERTION macro to _KUNIT_FAILED kunit: eliminate KUNIT_INIT_*_ASSERT_STRUCT macros kunit: declare kunit_assert structs as const
include/kunit/assert.h | 74 ++---------------------- include/kunit/test.h | 127 +++++++++++++++++++++++------------------ lib/kunit/test.c | 7 ++- 3 files changed, 80 insertions(+), 128 deletions(-)
base-commit: 511cce163b75bc3933fa3de769a82bb7e8663f2b
Each calll to a KUNIT_EXPECT_*() macro creates a local variable which contains a struct kunit_assert.
Normally, we'd hope the compiler would be able to optimize this away, but we've seen cases where it hasn't, see https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/GbrMNej2BAAJ.
In changes like commit 21957f90b28f ("kunit: split out part of kunit_assert into a static const"), we've moved more and more parts out of struct kunit_assert and its children types (kunit_binary_assert).
This patch removes the final field and gets us to: sizeof(struct kunit_assert) == 0 sizeof(struct kunit_binary_assert) == 24 (on UML x86_64).
This also reduces the amount of macro plumbing going on at the cost of passing in one more arg to the base KUNIT_ASSERTION macro and kunit_do_failed_assertion().
Signed-off-by: Daniel Latypov dlatypov@google.com --- include/kunit/assert.h | 28 ++++++---------------------- include/kunit/test.h | 17 +++++++++++------ lib/kunit/test.c | 7 ++++--- 3 files changed, 21 insertions(+), 31 deletions(-)
diff --git a/include/kunit/assert.h b/include/kunit/assert.h index 4b52e12c2ae8..ace3de8d1ee7 100644 --- a/include/kunit/assert.h +++ b/include/kunit/assert.h @@ -42,16 +42,15 @@ struct kunit_loc {
/** * struct kunit_assert - Data for printing a failed assertion or expectation. - * @format: a function which formats the data in this kunit_assert to a string. * * Represents a failed expectation/assertion. Contains all the data necessary to * format a string to a user reporting the failure. */ -struct kunit_assert { - void (*format)(const struct kunit_assert *assert, - const struct va_format *message, - struct string_stream *stream); -}; +struct kunit_assert {}; + +typedef void (*assert_format_t)(const struct kunit_assert *assert, + const struct va_format *message, + struct string_stream *stream);
void kunit_assert_prologue(const struct kunit_loc *loc, enum kunit_assert_type type, @@ -71,16 +70,6 @@ void kunit_fail_assert_format(const struct kunit_assert *assert, const struct va_format *message, struct string_stream *stream);
-/** - * KUNIT_INIT_FAIL_ASSERT_STRUCT - Initializer for &struct kunit_fail_assert. - * - * Initializes a &struct kunit_fail_assert. Intended to be used in - * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros. - */ -#define KUNIT_INIT_FAIL_ASSERT_STRUCT { \ - .assert = { .format = kunit_fail_assert_format }, \ -} - /** * struct kunit_unary_assert - Represents a KUNIT_{EXPECT|ASSERT}_{TRUE|FALSE} * @assert: The parent of this type. @@ -110,7 +99,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert, * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros. */ #define KUNIT_INIT_UNARY_ASSERT_STRUCT(cond, expect_true) { \ - .assert = { .format = kunit_unary_assert_format }, \ .condition = cond, \ .expected_true = expect_true \ } @@ -145,7 +133,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros. */ #define KUNIT_INIT_PTR_NOT_ERR_STRUCT(txt, val) { \ - .assert = { .format = kunit_ptr_not_err_assert_format }, \ .text = txt, \ .value = val \ } @@ -190,7 +177,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert, * KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a binary assert like * kunit_binary_assert, kunit_binary_ptr_assert, etc. * - * @format_func: a function which formats the assert to a string. * @text_: Pointer to a kunit_binary_assert_text. * @left_val: The actual evaluated value of the expression in the left slot. * @right_val: The actual evaluated value of the expression in the right slot. @@ -200,11 +186,9 @@ void kunit_binary_assert_format(const struct kunit_assert *assert, * fields but with different types for left_val/right_val. * This is ultimately used by binary assertion macros like KUNIT_EXPECT_EQ, etc. */ -#define KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func, \ - text_, \ +#define KUNIT_INIT_BINARY_ASSERT_STRUCT(text_, \ left_val, \ right_val) { \ - .assert = { .format = format_func }, \ .text = text_, \ .left_value = left_val, \ .right_value = right_val \ diff --git a/include/kunit/test.h b/include/kunit/test.h index 840a2c375065..3476549106f7 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -472,9 +472,10 @@ void kunit_do_failed_assertion(struct kunit *test, const struct kunit_loc *loc, enum kunit_assert_type type, const struct kunit_assert *assert, + assert_format_t assert_format, const char *fmt, ...);
-#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, INITIALIZER, fmt, ...) do { \ +#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, assert_format, INITIALIZER, fmt, ...) do { \ if (unlikely(!(pass))) { \ static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \ struct assert_class __assertion = INITIALIZER; \ @@ -482,6 +483,7 @@ void kunit_do_failed_assertion(struct kunit *test, &__loc, \ assert_type, \ &__assertion.assert, \ + assert_format, \ fmt, \ ##__VA_ARGS__); \ } \ @@ -493,7 +495,8 @@ void kunit_do_failed_assertion(struct kunit *test, assert_type, \ false, \ kunit_fail_assert, \ - KUNIT_INIT_FAIL_ASSERT_STRUCT, \ + kunit_fail_assert_format, \ + {}, \ fmt, \ ##__VA_ARGS__)
@@ -524,6 +527,7 @@ void kunit_do_failed_assertion(struct kunit *test, assert_type, \ !!(condition) == !!expected_true, \ kunit_unary_assert, \ + kunit_unary_assert_format, \ KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \ expected_true), \ fmt, \ @@ -581,8 +585,8 @@ do { \ assert_type, \ __left op __right, \ assert_class, \ - KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func, \ - &__text, \ + format_func, \ + KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ __left, \ __right), \ fmt, \ @@ -639,8 +643,8 @@ do { \ assert_type, \ strcmp(__left, __right) op 0, \ kunit_binary_str_assert, \ - KUNIT_INIT_BINARY_ASSERT_STRUCT(kunit_binary_str_assert_format,\ - &__text, \ + kunit_binary_str_assert_format, \ + KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ __left, \ __right), \ fmt, \ @@ -659,6 +663,7 @@ do { \ assert_type, \ !IS_ERR_OR_NULL(__ptr), \ kunit_ptr_not_err_assert, \ + kunit_ptr_not_err_assert_format, \ KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, \ __ptr), \ fmt, \ diff --git a/lib/kunit/test.c b/lib/kunit/test.c index b73d5bb5c473..53bf1793f915 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -247,7 +247,7 @@ static void kunit_print_string_stream(struct kunit *test,
static void kunit_fail(struct kunit *test, const struct kunit_loc *loc, enum kunit_assert_type type, const struct kunit_assert *assert, - const struct va_format *message) + assert_format_t assert_format, const struct va_format *message) { struct string_stream *stream;
@@ -263,7 +263,7 @@ static void kunit_fail(struct kunit *test, const struct kunit_loc *loc, }
kunit_assert_prologue(loc, type, stream); - assert->format(assert, message, stream); + assert_format(assert, message, stream);
kunit_print_string_stream(test, stream);
@@ -287,6 +287,7 @@ void kunit_do_failed_assertion(struct kunit *test, const struct kunit_loc *loc, enum kunit_assert_type type, const struct kunit_assert *assert, + assert_format_t assert_format, const char *fmt, ...) { va_list args; @@ -296,7 +297,7 @@ void kunit_do_failed_assertion(struct kunit *test, message.fmt = fmt; message.va = &args;
- kunit_fail(test, loc, type, assert, &message); + kunit_fail(test, loc, type, assert, assert_format, &message);
va_end(args);
On Sat, Oct 1, 2022 at 8:26 AM Daniel Latypov dlatypov@google.com wrote:
Each calll to a KUNIT_EXPECT_*() macro creates a local variable which contains a struct kunit_assert.
Normally, we'd hope the compiler would be able to optimize this away, but we've seen cases where it hasn't, see https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/GbrMNej2BAAJ.
In changes like commit 21957f90b28f ("kunit: split out part of kunit_assert into a static const"), we've moved more and more parts out of struct kunit_assert and its children types (kunit_binary_assert).
This patch removes the final field and gets us to: sizeof(struct kunit_assert) == 0 sizeof(struct kunit_binary_assert) == 24 (on UML x86_64).
This also reduces the amount of macro plumbing going on at the cost of passing in one more arg to the base KUNIT_ASSERTION macro and kunit_do_failed_assertion().
Signed-off-by: Daniel Latypov dlatypov@google.com
Very glad to see this finally happening: we've definitely still been seeing these stack size warnings, so reducing the stack use is good
And as someone who has always been a little uneasy with the number of nested macros, the simplification is also a big win.
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
include/kunit/assert.h | 28 ++++++---------------------- include/kunit/test.h | 17 +++++++++++------ lib/kunit/test.c | 7 ++++--- 3 files changed, 21 insertions(+), 31 deletions(-)
diff --git a/include/kunit/assert.h b/include/kunit/assert.h index 4b52e12c2ae8..ace3de8d1ee7 100644 --- a/include/kunit/assert.h +++ b/include/kunit/assert.h @@ -42,16 +42,15 @@ struct kunit_loc {
/**
- struct kunit_assert - Data for printing a failed assertion or expectation.
*/
- @format: a function which formats the data in this kunit_assert to a string.
- Represents a failed expectation/assertion. Contains all the data necessary to
- format a string to a user reporting the failure.
-struct kunit_assert {
void (*format)(const struct kunit_assert *assert,const struct va_format *message,struct string_stream *stream);-}; +struct kunit_assert {};
+typedef void (*assert_format_t)(const struct kunit_assert *assert,
const struct va_format *message,struct string_stream *stream);void kunit_assert_prologue(const struct kunit_loc *loc, enum kunit_assert_type type, @@ -71,16 +70,6 @@ void kunit_fail_assert_format(const struct kunit_assert *assert, const struct va_format *message, struct string_stream *stream);
-/**
- KUNIT_INIT_FAIL_ASSERT_STRUCT - Initializer for &struct kunit_fail_assert.
- Initializes a &struct kunit_fail_assert. Intended to be used in
- KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
- */
-#define KUNIT_INIT_FAIL_ASSERT_STRUCT { \
.assert = { .format = kunit_fail_assert_format }, \-}
/**
- struct kunit_unary_assert - Represents a KUNIT_{EXPECT|ASSERT}_{TRUE|FALSE}
- @assert: The parent of this type.
@@ -110,7 +99,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
- KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/ #define KUNIT_INIT_UNARY_ASSERT_STRUCT(cond, expect_true) { \
.assert = { .format = kunit_unary_assert_format }, \ .condition = cond, \ .expected_true = expect_true \} @@ -145,7 +133,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
- KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
*/ #define KUNIT_INIT_PTR_NOT_ERR_STRUCT(txt, val) { \
.assert = { .format = kunit_ptr_not_err_assert_format }, \ .text = txt, \ .value = val \} @@ -190,7 +177,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
- KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a binary assert like
kunit_binary_assert, kunit_binary_ptr_assert, etc.
- @format_func: a function which formats the assert to a string.
- @text_: Pointer to a kunit_binary_assert_text.
- @left_val: The actual evaluated value of the expression in the left slot.
- @right_val: The actual evaluated value of the expression in the right slot.
@@ -200,11 +186,9 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
- fields but with different types for left_val/right_val.
- This is ultimately used by binary assertion macros like KUNIT_EXPECT_EQ, etc.
*/ -#define KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func, \
text_, \+#define KUNIT_INIT_BINARY_ASSERT_STRUCT(text_, \ left_val, \ right_val) { \
.assert = { .format = format_func }, \ .text = text_, \ .left_value = left_val, \ .right_value = right_val \diff --git a/include/kunit/test.h b/include/kunit/test.h index 840a2c375065..3476549106f7 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -472,9 +472,10 @@ void kunit_do_failed_assertion(struct kunit *test, const struct kunit_loc *loc, enum kunit_assert_type type, const struct kunit_assert *assert,
assert_format_t assert_format, const char *fmt, ...);-#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, INITIALIZER, fmt, ...) do { \ +#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, assert_format, INITIALIZER, fmt, ...) do { \ if (unlikely(!(pass))) { \ static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \ struct assert_class __assertion = INITIALIZER; \ @@ -482,6 +483,7 @@ void kunit_do_failed_assertion(struct kunit *test, &__loc, \ assert_type, \ &__assertion.assert, \
assert_format, \ fmt, \ ##__VA_ARGS__); \ } \@@ -493,7 +495,8 @@ void kunit_do_failed_assertion(struct kunit *test, assert_type, \ false, \ kunit_fail_assert, \
KUNIT_INIT_FAIL_ASSERT_STRUCT, \
kunit_fail_assert_format, \{}, \ fmt, \ ##__VA_ARGS__)@@ -524,6 +527,7 @@ void kunit_do_failed_assertion(struct kunit *test, assert_type, \ !!(condition) == !!expected_true, \ kunit_unary_assert, \
kunit_unary_assert_format, \ KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \ expected_true), \ fmt, \@@ -581,8 +585,8 @@ do { \ assert_type, \ __left op __right, \ assert_class, \
KUNIT_INIT_BINARY_ASSERT_STRUCT(format_func, \&__text, \
format_func, \KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ __left, \ __right), \ fmt, \@@ -639,8 +643,8 @@ do { \ assert_type, \ strcmp(__left, __right) op 0, \ kunit_binary_str_assert, \
KUNIT_INIT_BINARY_ASSERT_STRUCT(kunit_binary_str_assert_format,\&__text, \
kunit_binary_str_assert_format, \KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ __left, \ __right), \ fmt, \@@ -659,6 +663,7 @@ do { \ assert_type, \ !IS_ERR_OR_NULL(__ptr), \ kunit_ptr_not_err_assert, \
kunit_ptr_not_err_assert_format, \ KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, \ __ptr), \ fmt, \diff --git a/lib/kunit/test.c b/lib/kunit/test.c index b73d5bb5c473..53bf1793f915 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -247,7 +247,7 @@ static void kunit_print_string_stream(struct kunit *test,
static void kunit_fail(struct kunit *test, const struct kunit_loc *loc, enum kunit_assert_type type, const struct kunit_assert *assert,
const struct va_format *message)
assert_format_t assert_format, const struct va_format *message){ struct string_stream *stream;
@@ -263,7 +263,7 @@ static void kunit_fail(struct kunit *test, const struct kunit_loc *loc, }
kunit_assert_prologue(loc, type, stream);
assert->format(assert, message, stream);
assert_format(assert, message, stream); kunit_print_string_stream(test, stream);@@ -287,6 +287,7 @@ void kunit_do_failed_assertion(struct kunit *test, const struct kunit_loc *loc, enum kunit_assert_type type, const struct kunit_assert *assert,
assert_format_t assert_format, const char *fmt, ...){ va_list args; @@ -296,7 +297,7 @@ void kunit_do_failed_assertion(struct kunit *test, message.fmt = fmt; message.va = &args;
kunit_fail(test, loc, type, assert, &message);
kunit_fail(test, loc, type, assert, assert_format, &message); va_end(args);-- 2.38.0.rc1.362.ged0d419d3c-goog
Context: Currently this macro's name, KUNIT_ASSERTION conflicts with the name of an enum whose values are {KUNIT_EXPECTATION, KUNIT_ASSERTION}.
It's hard to think of a better name for the enum, so rename this macro. It's also a bit strange that the macro might do nothing depending on the boolean argument `pass`. Why not have callers check themselves?
This patch: Moves the pass/fail checking into the callers of KUNIT_ASSERTION, so now we only call it when the check has failed. Then we rename the macro the _KUNIT_FAILED() to reflect the new semantics.
Signed-off-by: Daniel Latypov dlatypov@google.com --- include/kunit/test.h | 123 +++++++++++++++++++++++-------------------- 1 file changed, 65 insertions(+), 58 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 3476549106f7..fec437c8a2b7 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -475,30 +475,27 @@ void kunit_do_failed_assertion(struct kunit *test, assert_format_t assert_format, const char *fmt, ...);
-#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, assert_format, INITIALIZER, fmt, ...) do { \ - if (unlikely(!(pass))) { \ - static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \ - struct assert_class __assertion = INITIALIZER; \ - kunit_do_failed_assertion(test, \ - &__loc, \ - assert_type, \ - &__assertion.assert, \ - assert_format, \ - fmt, \ - ##__VA_ARGS__); \ - } \ +#define _KUNIT_FAILED(test, assert_type, assert_class, assert_format, INITIALIZER, fmt, ...) do { \ + static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \ + struct assert_class __assertion = INITIALIZER; \ + kunit_do_failed_assertion(test, \ + &__loc, \ + assert_type, \ + &__assertion.assert, \ + assert_format, \ + fmt, \ + ##__VA_ARGS__); \ } while (0)
#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) \ - KUNIT_ASSERTION(test, \ - assert_type, \ - false, \ - kunit_fail_assert, \ - kunit_fail_assert_format, \ - {}, \ - fmt, \ - ##__VA_ARGS__) + _KUNIT_FAILED(test, \ + assert_type, \ + kunit_fail_assert, \ + kunit_fail_assert_format, \ + {}, \ + fmt, \ + ##__VA_ARGS__)
/** * KUNIT_FAIL() - Always causes a test to fail when evaluated. @@ -523,15 +520,19 @@ void kunit_do_failed_assertion(struct kunit *test, expected_true, \ fmt, \ ...) \ - KUNIT_ASSERTION(test, \ - assert_type, \ - !!(condition) == !!expected_true, \ - kunit_unary_assert, \ - kunit_unary_assert_format, \ - KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \ - expected_true), \ - fmt, \ - ##__VA_ARGS__) +do { \ + if (likely(!!(condition) == !!expected_true)) \ + break; \ + \ + _KUNIT_FAILED(test, \ + assert_type, \ + kunit_unary_assert, \ + kunit_unary_assert_format, \ + KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \ + expected_true), \ + fmt, \ + ##__VA_ARGS__); \ +} while (0)
#define KUNIT_TRUE_MSG_ASSERTION(test, assert_type, condition, fmt, ...) \ KUNIT_UNARY_ASSERTION(test, \ @@ -581,16 +582,18 @@ do { \ .right_text = #right, \ }; \ \ - KUNIT_ASSERTION(test, \ - assert_type, \ - __left op __right, \ - assert_class, \ - format_func, \ - KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ - __left, \ - __right), \ - fmt, \ - ##__VA_ARGS__); \ + if (likely(__left op __right)) \ + break; \ + \ + _KUNIT_FAILED(test, \ + assert_type, \ + assert_class, \ + format_func, \ + KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ + __left, \ + __right), \ + fmt, \ + ##__VA_ARGS__); \ } while (0)
#define KUNIT_BINARY_INT_ASSERTION(test, \ @@ -639,16 +642,19 @@ do { \ .right_text = #right, \ }; \ \ - KUNIT_ASSERTION(test, \ - assert_type, \ - strcmp(__left, __right) op 0, \ - kunit_binary_str_assert, \ - kunit_binary_str_assert_format, \ - KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ - __left, \ - __right), \ - fmt, \ - ##__VA_ARGS__); \ + if (likely(strcmp(__left, __right) op 0)) \ + break; \ + \ + \ + _KUNIT_FAILED(test, \ + assert_type, \ + kunit_binary_str_assert, \ + kunit_binary_str_assert_format, \ + KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ + __left, \ + __right), \ + fmt, \ + ##__VA_ARGS__); \ } while (0)
#define KUNIT_PTR_NOT_ERR_OR_NULL_MSG_ASSERTION(test, \ @@ -659,15 +665,16 @@ do { \ do { \ const typeof(ptr) __ptr = (ptr); \ \ - KUNIT_ASSERTION(test, \ - assert_type, \ - !IS_ERR_OR_NULL(__ptr), \ - kunit_ptr_not_err_assert, \ - kunit_ptr_not_err_assert_format, \ - KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, \ - __ptr), \ - fmt, \ - ##__VA_ARGS__); \ + if (!IS_ERR_OR_NULL(__ptr)) \ + break; \ + \ + _KUNIT_FAILED(test, \ + assert_type, \ + kunit_ptr_not_err_assert, \ + kunit_ptr_not_err_assert_format, \ + KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, __ptr), \ + fmt, \ + ##__VA_ARGS__); \ } while (0)
/**
On Sat, Oct 1, 2022 at 8:26 AM 'Daniel Latypov' via KUnit Development kunit-dev@googlegroups.com wrote:
Context: Currently this macro's name, KUNIT_ASSERTION conflicts with the name of an enum whose values are {KUNIT_EXPECTATION, KUNIT_ASSERTION}.
It's hard to think of a better name for the enum, so rename this macro. It's also a bit strange that the macro might do nothing depending on the boolean argument `pass`. Why not have callers check themselves?
This patch: Moves the pass/fail checking into the callers of KUNIT_ASSERTION, so now we only call it when the check has failed. Then we rename the macro the _KUNIT_FAILED() to reflect the new semantics.
Signed-off-by: Daniel Latypov dlatypov@google.com
Looks good to me. I can't say the name _KUNIT_FAILED() feels perfect to me, but I can't think of anything better, either. We've not used a leading underscore for internal macros much thus far, as well, though I've no personal objections to starting.
Regardless, let's get this in.
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
include/kunit/test.h | 123 +++++++++++++++++++++++-------------------- 1 file changed, 65 insertions(+), 58 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 3476549106f7..fec437c8a2b7 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -475,30 +475,27 @@ void kunit_do_failed_assertion(struct kunit *test, assert_format_t assert_format, const char *fmt, ...);
-#define KUNIT_ASSERTION(test, assert_type, pass, assert_class, assert_format, INITIALIZER, fmt, ...) do { \
if (unlikely(!(pass))) { \static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \struct assert_class __assertion = INITIALIZER; \kunit_do_failed_assertion(test, \&__loc, \assert_type, \&__assertion.assert, \assert_format, \fmt, \##__VA_ARGS__); \} \+#define _KUNIT_FAILED(test, assert_type, assert_class, assert_format, INITIALIZER, fmt, ...) do { \
static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \struct assert_class __assertion = INITIALIZER; \kunit_do_failed_assertion(test, \&__loc, \assert_type, \&__assertion.assert, \assert_format, \fmt, \##__VA_ARGS__); \} while (0)
#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) \
KUNIT_ASSERTION(test, \assert_type, \false, \kunit_fail_assert, \kunit_fail_assert_format, \{}, \fmt, \##__VA_ARGS__)
_KUNIT_FAILED(test, \assert_type, \kunit_fail_assert, \kunit_fail_assert_format, \{}, \fmt, \##__VA_ARGS__)/**
- KUNIT_FAIL() - Always causes a test to fail when evaluated.
@@ -523,15 +520,19 @@ void kunit_do_failed_assertion(struct kunit *test, expected_true, \ fmt, \ ...) \
KUNIT_ASSERTION(test, \assert_type, \!!(condition) == !!expected_true, \kunit_unary_assert, \kunit_unary_assert_format, \KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \expected_true), \fmt, \##__VA_ARGS__)+do { \
if (likely(!!(condition) == !!expected_true)) \break; \\_KUNIT_FAILED(test, \assert_type, \kunit_unary_assert, \kunit_unary_assert_format, \KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \expected_true), \fmt, \##__VA_ARGS__); \+} while (0)
#define KUNIT_TRUE_MSG_ASSERTION(test, assert_type, condition, fmt, ...) \ KUNIT_UNARY_ASSERTION(test, \ @@ -581,16 +582,18 @@ do { \ .right_text = #right, \ }; \ \
KUNIT_ASSERTION(test, \assert_type, \__left op __right, \assert_class, \format_func, \KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \__left, \__right), \fmt, \##__VA_ARGS__); \
if (likely(__left op __right)) \break; \\_KUNIT_FAILED(test, \assert_type, \assert_class, \format_func, \KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \__left, \__right), \fmt, \##__VA_ARGS__); \} while (0)
#define KUNIT_BINARY_INT_ASSERTION(test, \ @@ -639,16 +642,19 @@ do { \ .right_text = #right, \ }; \ \
KUNIT_ASSERTION(test, \assert_type, \strcmp(__left, __right) op 0, \kunit_binary_str_assert, \kunit_binary_str_assert_format, \KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \__left, \__right), \fmt, \##__VA_ARGS__); \
if (likely(strcmp(__left, __right) op 0)) \break; \\\_KUNIT_FAILED(test, \assert_type, \kunit_binary_str_assert, \kunit_binary_str_assert_format, \KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \__left, \__right), \fmt, \##__VA_ARGS__); \} while (0)
#define KUNIT_PTR_NOT_ERR_OR_NULL_MSG_ASSERTION(test, \ @@ -659,15 +665,16 @@ do { \ do { \ const typeof(ptr) __ptr = (ptr); \ \
KUNIT_ASSERTION(test, \assert_type, \!IS_ERR_OR_NULL(__ptr), \kunit_ptr_not_err_assert, \kunit_ptr_not_err_assert_format, \KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, \__ptr), \fmt, \##__VA_ARGS__); \
if (!IS_ERR_OR_NULL(__ptr)) \break; \\_KUNIT_FAILED(test, \assert_type, \kunit_ptr_not_err_assert, \kunit_ptr_not_err_assert_format, \KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, __ptr), \fmt, \##__VA_ARGS__); \} while (0)
/**
2.38.0.rc1.362.ged0d419d3c-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/20221001002638.2881842-3-dlatypo....
On Fri, Sep 30, 2022 at 8:26 PM David Gow davidgow@google.com wrote:
On Sat, Oct 1, 2022 at 8:26 AM 'Daniel Latypov' via KUnit Development kunit-dev@googlegroups.com wrote:
Context: Currently this macro's name, KUNIT_ASSERTION conflicts with the name of an enum whose values are {KUNIT_EXPECTATION, KUNIT_ASSERTION}.
It's hard to think of a better name for the enum, so rename this macro. It's also a bit strange that the macro might do nothing depending on the boolean argument `pass`. Why not have callers check themselves?
This patch: Moves the pass/fail checking into the callers of KUNIT_ASSERTION, so now we only call it when the check has failed. Then we rename the macro the _KUNIT_FAILED() to reflect the new semantics.
Signed-off-by: Daniel Latypov dlatypov@google.com
Looks good to me. I can't say the name _KUNIT_FAILED() feels perfect to me, but I can't think of anything better, either. We've not used a leading underscore for internal macros much thus far, as well, though I've no personal objections to starting.
Yeah, I also didn't add a leading underscore on the new KUNIT_INIT_ASSERT() macro elsewhere in this series. So I'm not necessarily proposing that we should start doing so here.
It feels like that KUNIT_FAILED is far too similar to the enum 55 enum kunit_status { 56 KUNIT_SUCCESS, 57 KUNIT_FAILURE, 58 KUNIT_SKIPPED, 59 };
I.e. we'd be remove one naming conflict between a macro and enum, but basically introducing a new one in its place :P Tbh, I was originally going to have this patch just be s/KUNIT_ASSERTION()/_KUNIT_ASSERTION() to reduce the conflict. But I figured we could reduce the number of arguments to the macro (drop `pass`) and have a reason to give it a different name.
I'm also not entirely convinced about _KUNIT_FAILED(), but I haven't had any significantly better ideas since I sent the RFC in May.
Daniel
On Sat, Oct 1, 2022 at 11:50 AM 'Daniel Latypov' via KUnit Development kunit-dev@googlegroups.com wrote:
On Fri, Sep 30, 2022 at 8:26 PM David Gow davidgow@google.com wrote:
On Sat, Oct 1, 2022 at 8:26 AM 'Daniel Latypov' via KUnit Development kunit-dev@googlegroups.com wrote:
Context: Currently this macro's name, KUNIT_ASSERTION conflicts with the name of an enum whose values are {KUNIT_EXPECTATION, KUNIT_ASSERTION}.
It's hard to think of a better name for the enum, so rename this macro. It's also a bit strange that the macro might do nothing depending on the boolean argument `pass`. Why not have callers check themselves?
This patch: Moves the pass/fail checking into the callers of KUNIT_ASSERTION, so now we only call it when the check has failed. Then we rename the macro the _KUNIT_FAILED() to reflect the new semantics.
Signed-off-by: Daniel Latypov dlatypov@google.com
Looks good to me. I can't say the name _KUNIT_FAILED() feels perfect to me, but I can't think of anything better, either. We've not used a leading underscore for internal macros much thus far, as well, though I've no personal objections to starting.
Yeah, I also didn't add a leading underscore on the new KUNIT_INIT_ASSERT() macro elsewhere in this series. So I'm not necessarily proposing that we should start doing so here.
Yeah: I noticed that. In general, I think I come down slightly on the side of avoiding leading underscores. (And there's also the debate about whether to have one or two, as while two underscores is nominally "reserved for the system", the kernel uses it a lot -- probably because it considers itself "the system"). So I'd tend to lean away from making all of our "internal" macros start with underscores.
It feels like that KUNIT_FAILED is far too similar to the enum 55 enum kunit_status { 56 KUNIT_SUCCESS, 57 KUNIT_FAILURE, 58 KUNIT_SKIPPED, 59 };
I.e. we'd be remove one naming conflict between a macro and enum, but basically introducing a new one in its place :P Tbh, I was originally going to have this patch just be s/KUNIT_ASSERTION()/_KUNIT_ASSERTION() to reduce the conflict. But I figured we could reduce the number of arguments to the macro (drop `pass`) and have a reason to give it a different name.
I'm also not entirely convinced about _KUNIT_FAILED(), but I haven't had any significantly better ideas since I sent the RFC in May.
Agreed: particularly since SKIPPED would seem to pair better with FAILED than FAILURE, so they conflict quite a bit.
Let's stick with what we have in this change, and we can change it later if someone comes up with a drastically better name.
Cheers, -- David
These macros exist because passing an initializer list to other macros is hard.
The goal of these macros is to generate a line like struct $ASSERT_TYPE __assertion = $APPROPRIATE_INITIALIZER; e.g. struct kunit_unary_assertion __assertion = { .condition = "foo()", .expected_true = true };
But the challenge is you can't pass `{.condition=..., .expect_true=...}` as a macro argument, since the comma means you're actually passing two arguments, `{.condition=...` and `.expect_true=....}`. So we'd made custom macros for each different initializer-list shape.
But we can work around this with the following generic macro #define KUNIT_INIT_ASSERT(initializers...) { initializers }
Note: this has the downside that we have to rename some macros arguments to not conflict with the struct field names (e.g. `expected_true`). It's a bit gross, but probably worth reducing the # of macros.
Signed-off-by: Daniel Latypov dlatypov@google.com --- include/kunit/assert.h | 48 ------------------------------------------ include/kunit/test.h | 27 +++++++++++++----------- 2 files changed, 15 insertions(+), 60 deletions(-)
diff --git a/include/kunit/assert.h b/include/kunit/assert.h index ace3de8d1ee7..01b191fa17c3 100644 --- a/include/kunit/assert.h +++ b/include/kunit/assert.h @@ -90,19 +90,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert, const struct va_format *message, struct string_stream *stream);
-/** - * KUNIT_INIT_UNARY_ASSERT_STRUCT() - Initializes &struct kunit_unary_assert. - * @cond: A string representation of the expression asserted true or false. - * @expect_true: True if of type KUNIT_{EXPECT|ASSERT}_TRUE, false otherwise. - * - * Initializes a &struct kunit_unary_assert. Intended to be used in - * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros. - */ -#define KUNIT_INIT_UNARY_ASSERT_STRUCT(cond, expect_true) { \ - .condition = cond, \ - .expected_true = expect_true \ -} - /** * struct kunit_ptr_not_err_assert - An expectation/assertion that a pointer is * not NULL and not a -errno. @@ -123,20 +110,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, const struct va_format *message, struct string_stream *stream);
-/** - * KUNIT_INIT_PTR_NOT_ERR_ASSERT_STRUCT() - Initializes a - * &struct kunit_ptr_not_err_assert. - * @txt: A string representation of the expression passed to the expectation. - * @val: The actual evaluated pointer value of the expression. - * - * Initializes a &struct kunit_ptr_not_err_assert. Intended to be used in - * KUNIT_EXPECT_* and KUNIT_ASSERT_* macros. - */ -#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(txt, val) { \ - .text = txt, \ - .value = val \ -} - /** * struct kunit_binary_assert_text - holds strings for &struct * kunit_binary_assert and friends to try and make the structs smaller. @@ -173,27 +146,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert, const struct va_format *message, struct string_stream *stream);
-/** - * KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a binary assert like - * kunit_binary_assert, kunit_binary_ptr_assert, etc. - * - * @text_: Pointer to a kunit_binary_assert_text. - * @left_val: The actual evaluated value of the expression in the left slot. - * @right_val: The actual evaluated value of the expression in the right slot. - * - * Initializes a binary assert like kunit_binary_assert, - * kunit_binary_ptr_assert, etc. This relies on these structs having the same - * fields but with different types for left_val/right_val. - * This is ultimately used by binary assertion macros like KUNIT_EXPECT_EQ, etc. - */ -#define KUNIT_INIT_BINARY_ASSERT_STRUCT(text_, \ - left_val, \ - right_val) { \ - .text = text_, \ - .left_value = left_val, \ - .right_value = right_val \ -} - /** * struct kunit_binary_ptr_assert - An expectation/assertion that compares two * pointer values (for example, KUNIT_EXPECT_PTR_EQ(test, foo, bar)). diff --git a/include/kunit/test.h b/include/kunit/test.h index fec437c8a2b7..e49348bbc6ee 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -514,22 +514,25 @@ void kunit_do_failed_assertion(struct kunit *test, fmt, \ ##__VA_ARGS__)
+/* Helper to safely pass around an initializer list to other macros. */ +#define KUNIT_INIT_ASSERT(initializers...) { initializers } + #define KUNIT_UNARY_ASSERTION(test, \ assert_type, \ - condition, \ - expected_true, \ + condition_, \ + expected_true_, \ fmt, \ ...) \ do { \ - if (likely(!!(condition) == !!expected_true)) \ + if (likely(!!(condition_) == !!expected_true_)) \ break; \ \ _KUNIT_FAILED(test, \ assert_type, \ kunit_unary_assert, \ kunit_unary_assert_format, \ - KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \ - expected_true), \ + KUNIT_INIT_ASSERT(.condition = #condition_, \ + .expected_true = expected_true_), \ fmt, \ ##__VA_ARGS__); \ } while (0) @@ -589,9 +592,9 @@ do { \ assert_type, \ assert_class, \ format_func, \ - KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ - __left, \ - __right), \ + KUNIT_INIT_ASSERT(.text = &__text, \ + .left_value = __left, \ + .right_value = __right), \ fmt, \ ##__VA_ARGS__); \ } while (0) @@ -650,9 +653,9 @@ do { \ assert_type, \ kunit_binary_str_assert, \ kunit_binary_str_assert_format, \ - KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \ - __left, \ - __right), \ + KUNIT_INIT_ASSERT(.text = &__text, \ + .left_value = __left, \ + .right_value = __right), \ fmt, \ ##__VA_ARGS__); \ } while (0) @@ -672,7 +675,7 @@ do { \ assert_type, \ kunit_ptr_not_err_assert, \ kunit_ptr_not_err_assert_format, \ - KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, __ptr), \ + KUNIT_INIT_ASSERT(.text = #ptr, .value = __ptr), \ fmt, \ ##__VA_ARGS__); \ } while (0)
On Sat, Oct 1, 2022 at 8:26 AM Daniel Latypov dlatypov@google.com wrote:
These macros exist because passing an initializer list to other macros is hard.
The goal of these macros is to generate a line like struct $ASSERT_TYPE __assertion = $APPROPRIATE_INITIALIZER; e.g. struct kunit_unary_assertion __assertion = { .condition = "foo()", .expected_true = true };
But the challenge is you can't pass `{.condition=..., .expect_true=...}` as a macro argument, since the comma means you're actually passing two arguments, `{.condition=...` and `.expect_true=....}`. So we'd made custom macros for each different initializer-list shape.
But we can work around this with the following generic macro #define KUNIT_INIT_ASSERT(initializers...) { initializers }
Note: this has the downside that we have to rename some macros arguments to not conflict with the struct field names (e.g. `expected_true`). It's a bit gross, but probably worth reducing the # of macros.
Signed-off-by: Daniel Latypov dlatypov@google.com
I agree with you on both fronts here: this is 'a bit gross', and is also 'worth it'.
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
include/kunit/assert.h | 48 ------------------------------------------ include/kunit/test.h | 27 +++++++++++++----------- 2 files changed, 15 insertions(+), 60 deletions(-)
diff --git a/include/kunit/assert.h b/include/kunit/assert.h index ace3de8d1ee7..01b191fa17c3 100644 --- a/include/kunit/assert.h +++ b/include/kunit/assert.h @@ -90,19 +90,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert, const struct va_format *message, struct string_stream *stream);
-/**
- KUNIT_INIT_UNARY_ASSERT_STRUCT() - Initializes &struct kunit_unary_assert.
- @cond: A string representation of the expression asserted true or false.
- @expect_true: True if of type KUNIT_{EXPECT|ASSERT}_TRUE, false otherwise.
- Initializes a &struct kunit_unary_assert. Intended to be used in
- KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
- */
-#define KUNIT_INIT_UNARY_ASSERT_STRUCT(cond, expect_true) { \
.condition = cond, \.expected_true = expect_true \-}
/**
- struct kunit_ptr_not_err_assert - An expectation/assertion that a pointer is
not NULL and not a -errno.@@ -123,20 +110,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, const struct va_format *message, struct string_stream *stream);
-/**
- KUNIT_INIT_PTR_NOT_ERR_ASSERT_STRUCT() - Initializes a
&struct kunit_ptr_not_err_assert.
- @txt: A string representation of the expression passed to the expectation.
- @val: The actual evaluated pointer value of the expression.
- Initializes a &struct kunit_ptr_not_err_assert. Intended to be used in
- KUNIT_EXPECT_* and KUNIT_ASSERT_* macros.
- */
-#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(txt, val) { \
.text = txt, \.value = val \-}
/**
- struct kunit_binary_assert_text - holds strings for &struct
kunit_binary_assert and friends to try and make the structs smaller.@@ -173,27 +146,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert, const struct va_format *message, struct string_stream *stream);
-/**
- KUNIT_INIT_BINARY_ASSERT_STRUCT() - Initializes a binary assert like
kunit_binary_assert, kunit_binary_ptr_assert, etc.
- @text_: Pointer to a kunit_binary_assert_text.
- @left_val: The actual evaluated value of the expression in the left slot.
- @right_val: The actual evaluated value of the expression in the right slot.
- Initializes a binary assert like kunit_binary_assert,
- kunit_binary_ptr_assert, etc. This relies on these structs having the same
- fields but with different types for left_val/right_val.
- This is ultimately used by binary assertion macros like KUNIT_EXPECT_EQ, etc.
- */
-#define KUNIT_INIT_BINARY_ASSERT_STRUCT(text_, \
left_val, \right_val) { \.text = text_, \.left_value = left_val, \.right_value = right_val \-}
/**
- struct kunit_binary_ptr_assert - An expectation/assertion that compares two
pointer values (for example, KUNIT_EXPECT_PTR_EQ(test, foo, bar)).diff --git a/include/kunit/test.h b/include/kunit/test.h index fec437c8a2b7..e49348bbc6ee 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -514,22 +514,25 @@ void kunit_do_failed_assertion(struct kunit *test, fmt, \ ##__VA_ARGS__)
+/* Helper to safely pass around an initializer list to other macros. */ +#define KUNIT_INIT_ASSERT(initializers...) { initializers }
#define KUNIT_UNARY_ASSERTION(test, \ assert_type, \
condition, \expected_true, \
condition_, \expected_true_, \ fmt, \ ...) \do { \
if (likely(!!(condition) == !!expected_true)) \
if (likely(!!(condition_) == !!expected_true_)) \ break; \ \ _KUNIT_FAILED(test, \ assert_type, \ kunit_unary_assert, \ kunit_unary_assert_format, \
KUNIT_INIT_UNARY_ASSERT_STRUCT(#condition, \expected_true), \
KUNIT_INIT_ASSERT(.condition = #condition_, \.expected_true = expected_true_), \ fmt, \ ##__VA_ARGS__); \} while (0) @@ -589,9 +592,9 @@ do { \ assert_type, \ assert_class, \ format_func, \
KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \__left, \__right), \
KUNIT_INIT_ASSERT(.text = &__text, \.left_value = __left, \.right_value = __right), \ fmt, \ ##__VA_ARGS__); \} while (0) @@ -650,9 +653,9 @@ do { \ assert_type, \ kunit_binary_str_assert, \ kunit_binary_str_assert_format, \
KUNIT_INIT_BINARY_ASSERT_STRUCT(&__text, \__left, \__right), \
KUNIT_INIT_ASSERT(.text = &__text, \.left_value = __left, \.right_value = __right), \ fmt, \ ##__VA_ARGS__); \} while (0) @@ -672,7 +675,7 @@ do { \ assert_type, \ kunit_ptr_not_err_assert, \ kunit_ptr_not_err_assert_format, \
KUNIT_INIT_PTR_NOT_ERR_STRUCT(#ptr, __ptr), \
KUNIT_INIT_ASSERT(.text = #ptr, .value = __ptr), \ fmt, \ ##__VA_ARGS__); \} while (0)
2.38.0.rc1.362.ged0d419d3c-goog
On Sat, Oct 1, 2022 at 2:26 AM Daniel Latypov dlatypov@google.com wrote:
But we can work around this with the following generic macro #define KUNIT_INIT_ASSERT(initializers...) { initializers }
Is it intended to be internal, right? Should be prefixed by `_` then?
Cheers, Miguel
On Sat, Oct 1, 2022 at 3:12 AM Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Sat, Oct 1, 2022 at 2:26 AM Daniel Latypov dlatypov@google.com wrote:
But we can work around this with the following generic macro #define KUNIT_INIT_ASSERT(initializers...) { initializers }
Is it intended to be internal, right? Should be prefixed by `_` then?
Yeah, 100% internal.
We don't have such a convention in KUnit yet, see the discussion in https://lore.kernel.org/linux-kselftest/CABVgOSmcheQvBRKqc-0ftmbthx=EReoQ-91... I'd be personally fine with _s, but this patch just tried to keep things consistent with what was there before.
Daniel
Everywhere we use the assert structs now takes them via const*, as of commit 7466886b400b ("kunit: take `kunit_assert` as `const`").
So now let's properly declare the structs as const as well.
Signed-off-by: Daniel Latypov dlatypov@google.com --- include/kunit/test.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index e49348bbc6ee..d574c871dd9f 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -477,7 +477,7 @@ void kunit_do_failed_assertion(struct kunit *test,
#define _KUNIT_FAILED(test, assert_type, assert_class, assert_format, INITIALIZER, fmt, ...) do { \ static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \ - struct assert_class __assertion = INITIALIZER; \ + const struct assert_class __assertion = INITIALIZER; \ kunit_do_failed_assertion(test, \ &__loc, \ assert_type, \
On Sat, Oct 1, 2022 at 8:26 AM 'Daniel Latypov' via KUnit Development kunit-dev@googlegroups.com wrote:
Everywhere we use the assert structs now takes them via const*, as of commit 7466886b400b ("kunit: take `kunit_assert` as `const`").
So now let's properly declare the structs as const as well.
Signed-off-by: Daniel Latypov dlatypov@google.com
Thanks!
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
include/kunit/test.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index e49348bbc6ee..d574c871dd9f 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -477,7 +477,7 @@ void kunit_do_failed_assertion(struct kunit *test,
#define _KUNIT_FAILED(test, assert_type, assert_class, assert_format, INITIALIZER, fmt, ...) do { \ static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \
struct assert_class __assertion = INITIALIZER; \
const struct assert_class __assertion = INITIALIZER; \ kunit_do_failed_assertion(test, \ &__loc, \ assert_type, \-- 2.38.0.rc1.362.ged0d419d3c-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/20221001002638.2881842-5-dlatypo....
On Sat, Oct 1, 2022 at 2:26 AM Daniel Latypov dlatypov@google.com wrote:
Everywhere we use the assert structs now takes them via const*, as of commit 7466886b400b ("kunit: take `kunit_assert` as `const`").
So now let's properly declare the structs as const as well.
Always good to be `const`-correct :)
Reviewed-by: Miguel Ojeda ojeda@kernel.org
Cheers, Miguel
On Sat, Oct 1, 2022 at 2:26 AM Daniel Latypov dlatypov@google.com wrote:
Note: this does change the function signature of kunit_do_failed_assertion, so we'd need to update the rust wrapper in https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/kunit.rs, but hopefully it's just a simple change, e.g. maybe just like:
Yeah, should be simple. Thanks for pointing it out!
The series looks like a great cleanup on top of the stack reduction.
Cheers, Miguel
On Sat, Oct 1, 2022 at 3:15 AM Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Sat, Oct 1, 2022 at 2:26 AM Daniel Latypov dlatypov@google.com wrote:
Note: this does change the function signature of kunit_do_failed_assertion, so we'd need to update the rust wrapper in https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/kunit.rs, but hopefully it's just a simple change, e.g. maybe just like:
Yeah, should be simple. Thanks for pointing it out!
The series looks like a great cleanup on top of the stack reduction.
Thanks for taking a look at the rest of the series as well.
While I have you here, any thoughts on how to coordinate the change? I made the breaking change patch#1 so it should be easier to pull out.
One option I was thinking was: * wait till this lands in Shuah's tree * I create a Github PR that contains patch#1 + a patch for kunit.rs
I was not clear on how the RfL Github pulls in upstream changes or how often. But my assumption is patch#1 would fall away naturally if rebasing onto 6.1 (and maybe we can squash the kunit.rs change).
Thanks, Daniel
On Sat, Oct 1, 2022 at 8:00 PM Daniel Latypov dlatypov@google.com wrote:
While I have you here, any thoughts on how to coordinate the change?
My bad, I forgot to reply to this, sorry. I noticed it again when merging 6.1-rc1 into our branch.
Normally I fix the issues when I merge back from Linus. Since we intend to keep the CI green on every PR, I added the fix for this in the merge itself.
In any case, to clarify, the `rust` branch in GitHub is just where we placed a lot of things that we wanted to eventually submit (and some that should not, e.g. the GitHub CI files). We will be minimizing the differences w.r.t. upstream in that branch by preparing proper patches from scratch and submitting them through `rust-next` and other trees, and eventually remove it (or reusing the name for the fixes branch since that is the name other trees use, but we will see).
Cheers, Miguel
On Tue, Oct 18, 2022 at 4:20 PM Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Sat, Oct 1, 2022 at 8:00 PM Daniel Latypov dlatypov@google.com wrote:
While I have you here, any thoughts on how to coordinate the change?
My bad, I forgot to reply to this, sorry. I noticed it again when merging 6.1-rc1 into our branch.
No worries. You've had a very busy couple of weeks, I imagine.
Normally I fix the issues when I merge back from Linus. Since we intend to keep the CI green on every PR, I added the fix for this in the merge itself.
Thanks!
Daniel
On Wed, Oct 19, 2022 at 1:26 AM Daniel Latypov dlatypov@google.com wrote:
No worries. You've had a very busy couple of weeks, I imagine.
Just a bit :) Nevertheless, it was my intention to reply :(
I have linked this thread from the PR noting that you warned me about the future conflict [1], thanks again!
[1] https://github.com/Rust-for-Linux/linux/pull/915#issuecomment-1283138279
Cheers, Miguel
linux-kselftest-mirror@lists.linaro.org
-
Daniel Latypov -
David Gow -
Miguel Ojeda