Re: [PATCH 1/3] rust: macros: `parse_generics` add `decl_generics`
From: Jarkko Sakkinen
Date: Sat Nov 25 2023 - 09:08:47 EST
Sorry, just went through my eyes, hope you don't mind I nitpick
a bit. And maybe learn a bit in the process.
On Sat, 2023-11-25 at 12:50 +0000, Benno Lossin wrote:
> When parsing generics of a type definition, default values can be
> specified. This syntax is however only available on type definitions
> and
> not e.g. impl blocks.
Is "impl block" equivalent to a trait implementation? Maybe then just
write in better English "trait implementation? Would be IMHO better
to use commonly know terminology here.
Also for any commit, including any Rust commit. "When parsing" does
not map to anything concrete. There always should be a concrete
scenario where the parser its used. Especially since Rust is a new
thing in the kernel, these commits should really have more in-depth
information of the context.
I neither really grasped why trait implementations (if that is meant
by "impl block") not having this support connects to the code change.
Maybe just say that this patch adds the support and drop the whole
story about traits. It is sort of unnecessary context.
Finally, why this change is needed? Any commit should have existential
reason why it exists. So what will happen if "decl_generics" is not
taken to the upstream kernel? How does it make life more difficult?
You should be able to answer to this (in the commit message).
> This patch adds the `decl_generics` which can only be used on type
> defintions, since they contain the default values for generic
~~~~~~~~~~
definitions.
> parameters. This patch also changes how `impl_generics` are made up,
> as
> these should be used with `impl<$impl_generics>`, they will omit the
> default values.
What is decl_generics and what are the other _generics variables?
This lacks explanation what sort of change is implemented and why.
Nit:
s/This patch//:
1. "Add..." and
2. "Change...".
It is now useless pair of words and in the commit log "patch" is an
invalid word, given that it is a commit then, not a patch.
>
> Signed-off-by: Benno Lossin <benno.lossin@xxxxxxxxx>
> ---
> rust/macros/helpers.rs | 91 +++++++++++++++++++++++++++------------
> --
> rust/macros/pin_data.rs | 1 +
> rust/macros/zeroable.rs | 1 +
> 3 files changed, 63 insertions(+), 30 deletions(-)
>
> diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
> index afb0f2e3a36a..36fecdd998d0 100644
> --- a/rust/macros/helpers.rs
> +++ b/rust/macros/helpers.rs
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
>
> -use proc_macro::{token_stream, Group, Punct, Spacing, TokenStream,
> TokenTree};
> +use proc_macro::{token_stream, Group, TokenStream, TokenTree};
>
> pub(crate) fn try_ident(it: &mut token_stream::IntoIter) ->
> Option<String> {
> if let Some(TokenTree::Ident(ident)) = it.next() {
> @@ -72,6 +72,7 @@ pub(crate) fn expect_end(it: &mut
> token_stream::IntoIter) {
>
> pub(crate) struct Generics {
> pub(crate) impl_generics: Vec<TokenTree>,
> + pub(crate) decl_generics: Vec<TokenTree>,
> pub(crate) ty_generics: Vec<TokenTree>,
> }
>
> @@ -81,6 +82,8 @@ pub(crate) struct Generics {
> pub(crate) fn parse_generics(input: TokenStream) -> (Generics,
> Vec<TokenTree>) {
> // `impl_generics`, the declared generics with their bounds.
> let mut impl_generics = vec![];
> + // The generics with bounds and default values.
> + let mut decl_generics = vec![];
> // Only the names of the generics, without any bounds.
> let mut ty_generics = vec![];
> // Tokens not related to the generics e.g. the `where` token and
> definition.
> @@ -90,10 +93,17 @@ pub(crate) fn parse_generics(input: TokenStream)
> -> (Generics, Vec<TokenTree>) {
> let mut toks = input.into_iter();
> // If we are at the beginning of a generic parameter.
> let mut at_start = true;
> - for tt in &mut toks {
> + let mut skip_until_comma = false;
> + while let Some(tt) = toks.next() {
> + if nesting == 1 && matches!(&tt, TokenTree::Punct(p) if
> p.as_char() == '>') {
> + // Found the end of the generics.
> + break;
> + } else if nesting >= 1 {
> + decl_generics.push(tt.clone());
> + }
> match tt.clone() {
> TokenTree::Punct(p) if p.as_char() == '<' => {
> - if nesting >= 1 {
> + if nesting >= 1 && !skip_until_comma {
> // This is inside of the generics and part of
> some bound.
> impl_generics.push(tt);
> }
> @@ -105,49 +115,70 @@ pub(crate) fn parse_generics(input:
> TokenStream) -> (Generics, Vec<TokenTree>) {
> break;
> } else {
> nesting -= 1;
> - if nesting >= 1 {
> + if nesting >= 1 && !skip_until_comma {
> // We are still inside of the generics and
> part of some bound.
> impl_generics.push(tt);
> }
> - if nesting == 0 {
> - break;
> - }
> }
> }
> - tt => {
> + TokenTree::Punct(p) if skip_until_comma && p.as_char()
> == ',' => {
> if nesting == 1 {
> - // Here depending on the token, it might be a
> generic variable name.
> - match &tt {
> - // Ignore const.
> - TokenTree::Ident(i) if i.to_string() ==
> "const" => {}
> - TokenTree::Ident(_) if at_start => {
> - ty_generics.push(tt.clone());
> - // We also already push the `,` token,
> this makes it easier to append
> - // generics.
> -
> ty_generics.push(TokenTree::Punct(Punct::new(',', Spacing::Alone)));
> - at_start = false;
> - }
> - TokenTree::Punct(p) if p.as_char() == ',' =>
> at_start = true,
> - // Lifetimes begin with `'`.
> - TokenTree::Punct(p) if p.as_char() == '\''
> && at_start => {
> - ty_generics.push(tt.clone());
> - }
> - _ => {}
> - }
> + impl_generics.push(TokenTree::Punct(p.clone()));
> + ty_generics.push(TokenTree::Punct(p));
> + skip_until_comma = false;
> }
> - if nesting >= 1 {
> - impl_generics.push(tt);
> - } else if nesting == 0 {
> + }
> + tt if !skip_until_comma => {
> + match nesting {
> // If we haven't entered the generics yet, we
> still want to keep these tokens.
> - rest.push(tt);
> + 0 => rest.push(tt),
> + 1 => {
> + // Here depending on the token, it might be
> a generic variable name.
> + match tt {
> + TokenTree::Ident(i) if at_start &&
> i.to_string() == "const" => {
> + let Some(name) = toks.next() else {
> + // Parsing error.
> + break;
> + };
> +
> impl_generics.push(TokenTree::Ident(i));
> + impl_generics.push(name.clone());
> + ty_generics.push(name.clone());
> + decl_generics.push(name);
> + at_start = false;
> + }
> + tt @ TokenTree::Ident(_) if at_start =>
> {
> + impl_generics.push(tt.clone());
> + ty_generics.push(tt);
> + at_start = false;
> + }
> + TokenTree::Punct(p) if p.as_char() ==
> ',' => {
> +
> impl_generics.push(TokenTree::Punct(p.clone()));
> +
> ty_generics.push(TokenTree::Punct(p));
> + at_start = true;
> + }
> + // Lifetimes begin with `'`.
> + TokenTree::Punct(p) if p.as_char() ==
> '\'' && at_start => {
> +
> ty_generics.push(TokenTree::Punct(p.clone()));
> +
> impl_generics.push(TokenTree::Punct(p));
> + }
> + // Generics can have default values, we
> skip these.
> + TokenTree::Punct(p) if p.as_char() ==
> '=' => {
> + skip_until_comma = true;
> + }
> + tt => impl_generics.push(tt),
> + }
> + }
> + _ => impl_generics.push(tt),
> }
> }
> + _ => {}
> }
> }
> rest.extend(toks);
> (
> Generics {
> impl_generics,
> + decl_generics,
> ty_generics,
> },
> rest,
> diff --git a/rust/macros/pin_data.rs b/rust/macros/pin_data.rs
> index 6d58cfda9872..022e68e9720d 100644
> --- a/rust/macros/pin_data.rs
> +++ b/rust/macros/pin_data.rs
> @@ -10,6 +10,7 @@ pub(crate) fn pin_data(args: TokenStream, input:
> TokenStream) -> TokenStream {
> let (
> Generics {
> impl_generics,
> + decl_generics: _,
> ty_generics,
> },
> rest,
> diff --git a/rust/macros/zeroable.rs b/rust/macros/zeroable.rs
> index 0d605c46ab3b..cfee2cec18d5 100644
> --- a/rust/macros/zeroable.rs
> +++ b/rust/macros/zeroable.rs
> @@ -7,6 +7,7 @@ pub(crate) fn derive(input: TokenStream) ->
> TokenStream {
> let (
> Generics {
> impl_generics,
> + decl_generics: _,
> ty_generics,
> },
> mut rest,
>
> base-commit: 98b1cc82c4affc16f5598d4fa14b1858671b2263
BR, Jarkko