On Fri, Dec 13, 2024 at 9:10 AM David Gow <davidgow@xxxxxxxxxx> 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@xxxxxxxxx>
(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
