On Fri, Dec 13, 2024 at 9:10 AM David Gow davidgow@google.com wrote:
#[allow(unused_unsafe)]
Should this be in the first patch?
unsafe { core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) };
unsafe {core::ptr::addr_of_mut!(KUNIT_TEST_SUITE)};
Spurious change?
I thought this should perhaps have been in the previous patch directly, but running `rustfmt` shows the previous patch is the correct formatting.
`rustfmt` also needs to be run for these files -- it reveals a few more changes needed.
+//! Copyright (c) 2023 José Expósito jose.exposito89@gmail.com
(This is not something we need to change now, since it is orthogonal and debatable, but I think copyright lines should not be in the generated documentation unless we really want contact points in docs. I think it could go in a `//` comment after the `SPDX` line instead.)
+pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream {
- if attr.to_string().is_empty() {
panic!("Missing test name in #[kunit_tests(test_name)] macro")- }
- if attr.to_string().as_bytes().len() > 255 {
panic!("The test suite name `{}` exceeds the maximum length of 255 bytes.", attr)- }
We could do the `to_string()` step once creating a single string (and we can keep it as a `String`, too):
let attr = attr.to_string();
- // Scan for the "mod" keyword.
Formatting: `mod`
.expect("#[kunit_tests(test_name)] attribute should only be applied to modules");
Formatting: `#[kunit_tests(test_name)]`
_ => panic!("cannot locate main body of module"),
Formatting: Cannot
- // #[test]
- // fn bar() {
- // assert_eq!(2, 2);
- // }
- // ```
Missing closing `}` of the `mod`.
- // static mut KUNIT_CASE_FOO: kernel::bindings::kunit_case =
- // kernel::kunit::kunit_case(foo, kunit_rust_wrapper_foo);
I think this is not in sync with the generation, i.e. missing kernel::c_str!("foo")
(and ditto for the other one)
- // static mut TEST_CASES : &mut[kernel::bindings::kunit_case] = unsafe {
Formatting: extra space and missing space.
- // &mut [KUNIT_CASE_FOO, KUNIT_CASE_BAR, KUNIT_CASE_NULL]
- // };
- //
- // kernel::kunit_unsafe_test_suite!(kunit_test_suit_name, TEST_CASES);
- // ```
With the suggestions from the previous patch (i.e. avoiding unused/duplicated `static`s, intermediate mutable references, unstable feature and `unsafe`), we can simplify the entire example down to:
// ``` // unsafe extern "C" fn kunit_rust_wrapper_foo(_test: *mut kernel::bindings::kunit) { foo(); } // unsafe extern "C" fn kunit_rust_wrapper_bar(_test: * mut kernel::bindings::kunit) { bar(); } // // static mut TEST_CASES: [kernel::bindings::kunit_case; 3] = [ // kernel::kunit::kunit_case(kernel::c_str!("foo"), kunit_rust_wrapper_foo), // kernel::kunit::kunit_case(kernel::c_str!("bar"), kunit_rust_wrapper_bar), // kernel::kunit::kunit_case_null() // ]; // // kernel::kunit_unsafe_test_suite!(kunit_test_suit_name, TEST_CASES); // ```
...with the corresponding removals in the actual generation code below, of course.
Cheers, Miguel