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: <ce55240f74d4630fef8c7c3e7f47fe1b09636da1.camel@kernel.org>
Date:   Sat, 25 Nov 2023 15:39:10 +0200
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     Benno Lossin <benno.lossin@...ton.me>,
        Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        Andreas Hindborg <a.hindborg@...sung.com>,
        Alice Ryhl <aliceryhl@...gle.com>,
        Martin Rodriguez Reboredo <yakoyoku@...il.com>,
        Asahi Lina <lina@...hilina.net>
Cc:     Sumera Priyadarsini <sylphrenadin@...il.com>,
        rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] rust: macros: `parse_generics` add `decl_generics`

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@...ton.me>
> ---
>  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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ