linux-kernel - Re: [PATCH 1/2] rust: macros: allow optional trailing comma in module!

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87zfeamd8c.fsf@posteo.net>
Date: Sat, 14 Jun 2025 17:42:27 +0000
From: Charalampos Mitrodimas <charmitro@...teo.net>
To: Eunsoo Eun <ewhk9887@...il.com>
Cc: rust-for-linux@...r.kernel.org,  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!

Eunsoo Eun <ewhk9887@...il.com> writes:

> 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);
> +    

We have some whitespaces here ^

> +    // Check for optional trailing comma
> +    if let Some(TokenTree::Punct(punct)) = it.clone().next() {
> +        if punct.as_char() == ',' {
> +            // Consume the trailing comma
> +            it.next();
> +        }
> +    }
> +    

Whitespaces also here ^.

Maybe you can add a new helper function for this one?

>      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() {

We might be able to do something like this here,
   let next_token = it.clone().next();
   match next_token {
         ...

> +            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;
> +                }

Lose this, and let it check naturally?

> +            }
> +            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
> +                }
> +            }
>          }
>  
>          expect_end(it);