Re: [PATCH v3 06/16] rust: io: register: allow explicit base type specification

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `register!` macro's parsing logic for the optional `base:` parameter creates an ambiguity that breaks parsing if the first register is named `base`.
- [Medium] Discarding the `base` parameter in the macro output causes `unused_import` warnings in updated doc tests, breaking `make rusttest`.
--

--- Patch [6]: [PATCH v3 06/16] rust: io: register: allow explicit base type specification ---
commit 35780b69bb052cc1fae4709e732645521408cd48
Author: Gary Guo <[email protected]>

rust: io: register: allow explicit base type specification

This commit modifies the register! macro to accept an optional base
parameter to specify the base type it will be operating on. Currently,
the parameter is parsed but ignored in the generated code until all
users are converted.

> diff --git a/rust/kernel/io/register.rs b/rust/kernel/io/register.rs
> index 45d2e73a92e7e..49a5fe069e47f 100644
> --- a/rust/kernel/io/register.rs
> +++ b/rust/kernel/io/register.rs
> @@ -13,9 +13,14 @@
>  //! # Simple example
>  //!
>  //! ```no_run
> -//! use kernel::io::register;
> +//! use kernel::io::{
> +//!     register,
> +//!     Region,
> +//! };
>  //!
>  //! register! {
> +//!     base: Region<0x1000>;
> +//!

[Severity: Medium]
Will this result in an unused import warning that breaks make rusttest?

Since the base parameter is parsed but entirely discarded from the emitted
token stream, rustc will never see Region being used. The kernel compiles doc
tests with -Dwarnings enabled, so this unused import could cause the doc
tests to fail.

[ ... ]

> diff --git a/rust/macros/io/register.rs b/rust/macros/io/register.rs
> index 8af12caa964b8..2fb48e1be82d6 100644
> --- a/rust/macros/io/register.rs
> +++ b/rust/macros/io/register.rs

[ ... ]

> @@ -139,16 +140,26 @@ fn parse(input: syn::parse::ParseStream<'_>) -> Result<Self> {
>  }
>  
>  pub(crate) struct RegDef {
> +    base: Option<Type>,
>      regs: Vec<Reg>,
>  }
>  
>  impl Parse for RegDef {
>      fn parse(input: syn::parse::ParseStream<'_>) -> Result<Self> {
> +        let base = if input.peek(kw::base) {
> +            let _: kw::base = input.parse()?;
> +            let _: Token![:] = input.parse()?;

[Severity: Medium]
Does this parsing logic break if a user names their first register base?

By only using input.peek(kw::base), the macro eagerly matches any identifier
named base at the start of the block. If a user defines a register like
base(u32) @ 0x100 (without a visibility modifier), it seems the macro will
consume base, expect a colon, and then fail with a syntax error when it
encounters the parenthesis.

Would checking for the trailing colon using input.peek2(Token![:]) prevent
this ambiguity?

> +            let base = input.parse()?;
> +            let _: Token![;] = input.parse()?;
> +            Some(base)
> +        } else {
> +            None
> +        };
>          let mut regs = Vec::new();
>          while !input.is_empty() {
>              regs.push(input.parse()?);
>          }
> -        Ok(RegDef { regs })
> +        Ok(RegDef { base, regs })
>      }
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6