Re: [PATCH v12 2/3] rust: add parameter support to the `module!` macro

[Andreas Hindborg <a.hindborg@kernel.org>]

"Benno Lossin" <lossin@kernel.org> writes:

> On Wed Jun 11, 2025 at 12:31 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Tue May 6, 2025 at 3:02 PM CEST, Andreas Hindborg wrote:
>>>> diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
>>>> index a3ee27e29a6f..16d300ad3d3b 100644
>>>> --- a/rust/macros/helpers.rs
>>>> +++ b/rust/macros/helpers.rs
>>>> @@ -10,6 +10,17 @@ pub(crate) fn try_ident(it: &mut token_stream::IntoIter) -> Option<String> {
>>>>      }
>>>>  }
>>>>
>>>> +pub(crate) fn try_sign(it: &mut token_stream::IntoIter) -> Option<char> {
>>>> +    let peek = it.clone().next();
>>>> +    match peek {
>>>> +        Some(TokenTree::Punct(punct)) if punct.as_char() == '-' => {
>>>
>>> Should we also allow a leading `+`?
>>
>> I would argue no, because rust literals cannot start with `+`.
>
> Makes sense.
>
>>>> +            let _ = it.next();
>>>> +            Some(punct.as_char())
>>>> +        }
>>>> +        _ => None,
>>>> +    }
>>>> +}
>>>> +
>>>>  pub(crate) fn try_literal(it: &mut token_stream::IntoIter) -> Option<String> {
>>>>      if let Some(TokenTree::Literal(literal)) = it.next() {
>>>>          Some(literal.to_string())
>>>> @@ -86,3 +97,17 @@ pub(crate) fn function_name(input: TokenStream) -> Option<Ident> {
>>>>      }
>>>>      None
>>>>  }
>>>> +
>>>> +/// Parse a token stream of the form `expected_name: "value",` and return the
>>>> +/// string in the position of "value".
>>>> +///
>>>> +/// # Panics
>>>> +///
>>>> +/// - On parse error.
>>>> +pub(crate) fn expect_string_field(it: &mut token_stream::IntoIter, expected_name: &str) -> String {
>>>> +    assert_eq!(expect_ident(it), expected_name);
>>>> +    assert_eq!(expect_punct(it), ':');
>>>> +    let string = expect_string(it);
>>>> +    assert_eq!(expect_punct(it), ',');
>>>
>>> This won't allow omitting the trailing comma.
>>
>> This is in line with the rest of the module macro.
>
> Then we should change that:
>
>     https://github.com/Rust-for-Linux/linux/issues/1172
>
>>>> +    string
>>>> +}
>>>
>>> [...]
>>>
>>>> @@ -186,33 +336,35 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>>>>      let info = ModuleInfo::parse(&mut it);
>>>>
>>>>      let mut modinfo = ModInfoBuilder::new(info.name.as_ref());
>>>> -    if let Some(author) = info.author {
>>>> -        modinfo.emit("author", &author);
>>>> +    if let Some(author) = &info.author {
>>>> +        modinfo.emit("author", author);
>>>>      }
>>>> -    if let Some(authors) = info.authors {
>>>> +    if let Some(authors) = &info.authors {
>>>>          for author in authors {
>>>> -            modinfo.emit("author", &author);
>>>> +            modinfo.emit("author", author);
>>>>          }
>>>>      }
>>>> -    if let Some(description) = info.description {
>>>> -        modinfo.emit("description", &description);
>>>> +    if let Some(description) = &info.description {
>>>> +        modinfo.emit("description", description);
>>>>      }
>>>>      modinfo.emit("license", &info.license);
>>>> -    if let Some(aliases) = info.alias {
>>>> +    if let Some(aliases) = &info.alias {
>>>>          for alias in aliases {
>>>> -            modinfo.emit("alias", &alias);
>>>> +            modinfo.emit("alias", alias);
>>>>          }
>>>>      }
>>>> -    if let Some(firmware) = info.firmware {
>>>> +    if let Some(firmware) = &info.firmware {
>>>>          for fw in firmware {
>>>> -            modinfo.emit("firmware", &fw);
>>>> +            modinfo.emit("firmware", fw);
>>>
>>> I don't like that you have to change all of these.
>>
>> Why not? If I was to write this code in the first place, I would have
>> used a reference rather than pass by value.
>
> That's fine, but do it in a separate commit then.

OK, I can do that 👍


Best regards,
Andreas Hindborg