Message-Id: <DAMGM426FUU1.2IESY59SPIIHN@kernel.org>
Date: Sat, 14 Jun 2025 20:15:24 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Eunsoo Eun" <ewhk9887@...il.com>, <rust-for-linux@...r.kernel.org>
Cc: <linux-kernel@...r.kernel.org>, "Eunsoo Eun" <naturale@...s.ac.kr>,
 "Benno Lossin" <benno.lossin@...ton.me>
Subject: Re: [PATCH 1/2] rust: macros: allow optional trailing comma in
 module!

I didn't get the second patch advertised in the subject, did something
go wrong when sending the patch?

On Sat Jun 14, 2025 at 10:13 AM CEST, Eunsoo Eun wrote:
> From: Eunsoo Eun <naturale@...s.ac.kr>
>
> Make the `module!` macro syntax more flexible by allowing an optional
> trailing comma after the last field. This makes it consistent with
> Rust’s general syntax patterns where trailing commas are allowed in
> structs, arrays, and other comma-separated lists.
>
> For example, these are now all valid:
>
>     module! {
>         type: MyModule,
>         name: "mymodule",
>         license: "GPL"  // No trailing comma
>     }
>
>     module! {
>         type: MyModule,
>         name: "mymodule",
>         license: "GPL",  // With trailing comma
>     }
>
> This change also allows optional trailing commas in array fields like
> `authors`, `alias`, and `firmware`:
>
>     module! {
>         type: MyModule,
>         name: "mymodule",
>         authors: ["Author 1", "Author 2"],  // No trailing comma
>         license: "GPL"
>     }
>
>     module! {
>         type: MyModule,
>         name: "mymodule",
>         authors: ["Author 1", "Author 2",], // With trailing comma
>         license: "GPL"
>     }
>
> Suggested-by: Benno Lossin <benno.lossin@...ton.me>
> Link: https://github.com/Rust-for-Linux/linux/issues/1172
> Signed-off-by: Eunsoo Eun <naturale@...s.ac.kr>
> ---
>  rust/macros/concat_idents.rs |  9 ++++++++
>  rust/macros/module.rs        | 42 ++++++++++++++++++++++++++++++------
>  2 files changed, 45 insertions(+), 6 deletions(-)
>
> diff --git a/rust/macros/concat_idents.rs b/rust/macros/concat_idents.rs
> index 7e4b450f3a50..c139e1658b4a 100644
> --- a/rust/macros/concat_idents.rs
> +++ b/rust/macros/concat_idents.rs
> @@ -17,6 +17,15 @@ pub(crate) fn concat_idents(ts: TokenStream) -> TokenStream {
>      let a = expect_ident(&mut it);
>      assert_eq!(expect_punct(&mut it), ',');
>      let b = expect_ident(&mut it);
> +    
> +    // Check for optional trailing comma
> +    if let Some(TokenTree::Punct(punct)) = it.clone().next() {

Please avoid cloning the iterator, as that is inefficient. Instead use
`peekable()` to create an iterator with a `peek` function.

> +        if punct.as_char() == ',' {
> +            // Consume the trailing comma
> +            it.next();
> +        }

This allows any kind of trailing token in concat_idents, but only a
comma should be allowed.

> +    }
> +    
>      assert!(it.next().is_none(), "only two idents can be concatenated");
>      let res = Ident::new(&format!("{a}{b}"), b.span());
>      TokenStream::from_iter([TokenTree::Ident(res)])
> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> index 2ddd2eeb2852..d37492457be5 100644
> --- a/rust/macros/module.rs
> +++ b/rust/macros/module.rs
> @@ -13,10 +13,27 @@ fn expect_string_array(it: &mut token_stream::IntoIter) -> Vec<String> {
>      while let Some(val) = try_string(&mut it) {
>          assert!(val.is_ascii(), "Expected ASCII string");
>          values.push(val);
> -        match it.next() {
> -            Some(TokenTree::Punct(punct)) => assert_eq!(punct.as_char(), ','),
> -            None => break,
> -            _ => panic!("Expected ',' or end of array"),
> +
> +        // Check for optional trailing comma
> +        match it.clone().next() {

Please avoid cloning the iterator, as that is inefficient. Instead use
`peekable()` to create an iterator with a `peek` function.

> +            Some(TokenTree::Punct(punct)) if punct.as_char() == ',' => {
> +                // Consume the comma
> +                it.next();
> +                // Check if there's another string after the comma
> +                if it.clone().next().is_none() {
> +                    // Trailing comma at end of array is allowed
> +                    break;
> +                }
> +            }
> +            Some(TokenTree::Literal(_)) => {
> +                // Next item is a string literal, comma was required
> +                panic!("Expected ',' between array elements");
> +            }
> +            None => {
> +                // End of array, no comma needed
> +                break;
> +            }
> +            Some(_) => panic!("Expected ',' or end of array"),
>          }
>      }
>      values
> @@ -143,9 +160,22 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
>                  _ => panic!("Unknown key \"{key}\". Valid keys are: {EXPECTED_KEYS:?}."),
>              }
>  
> -            assert_eq!(expect_punct(it), ',');
> -
>              seen_keys.push(key);
> +
> +            // Check for optional trailing comma
> +            match it.clone().next() {
> +                Some(TokenTree::Punct(punct)) if punct.as_char() == ',' => {
> +                    // Consume the comma
> +                    it.next();
> +                }
> +                Some(TokenTree::Ident(_)) => {
> +                    // Next item is an identifier, comma was required
> +                    panic!("Expected ',' between module properties");
> +                }
> +                _ => {
> +                    // End of input or closing brace, comma is optional

This also could be a new opening group or other punctuation, which would
be wrong.

---
Cheers,
Benno

> +                }
> +            }
>          }
>  
>          expect_end(it);

Powered by blists - more mailing lists