Message-Id: <CXE115I7V0YC.2AZ9KSVZP4NVS@kernel.org>
Date:   Sat, 02 Dec 2023 19:33:45 +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`

On Sat Nov 25, 2023 at 5:39 PM EET, Benno Lossin wrote:
> On 25.11.23 14:39, Jarkko Sakkinen wrote:
> > 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.
>
> "impl block" refers to the syntactic item of Implementation [1]. It
> might be a trait implementation, or an inherent implementation. To me
> "impl block" is known terminology.
>
> [1]: https://doc.rust-lang.org/stable/reference/items/implementations.html
>
> > 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.
>
> This commit is tagged `rust: macros:`, which means that it affects the
> proc macros. So when I wrote "When parsing", I meant "When parsing Rust
> code in proc macros". I will change this for v2.
>
> > 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.
>
> Rust does not syntactically support writing
>
>      impl<const N: usize = 0> Foo<N> {
>      }
>
> This is because it does not make sense. The syntax `= 0` only makes
> sense on type definitions:
>
>      struct Foo<const N: usize = 0> {
>      }
>
> Because then you can just write `Foo` and it will be the same type as
> `Foo<0>`.

Right.

>
> > 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).
>
> Does this explain it?:
>
> In order to allow `#[pin_data]` on structs with default values for const
> generic parameters, the `#[pin_data]` macro needs to parse them and have
> access to the generics as they are written on the type definition.
> This commit adds support for parsing them to the already present generics
> parsing code in the macros crate.

Yes.

>
> >> 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.
>
> The terms `impl_generics` and `ty_generics` are taken from [2]. This
> patch adds a third kind which also contains any default values of const
> generic parameters. I named them `decl_generics`, because they only
> appear on type declarations.
>
> [2]: https://docs.rs/syn/latest/syn/struct.Generics.html#method.split_for_impl

Thanks a lot of taking time for explaining all these concepts in a such
a detail. Appreciate it! And I apologize for my a bit intrusive
response.

I really think that "more vocal and verbose" than "legacy C" patches
would be a great policy for Rust specific patches. This would help
audience who understand kernel but are not as in Rust to give more
feedback on the patches. I mean tech is still the same whatever we
used to implement the code that enables it.

By doing that I see that all benefit and it opens doors for deeper
Rust integration in the kernel.

BR, Jarkko

Powered by blists - more mailing lists