"Benno Lossin" <los...@kernel.org> writes:
> On Wed Jun 11, 2025 at 12:31 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <los...@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
