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
