Message-ID: <84da128f-17b3-4193-8389-176a8c961d64@proton.me>
Date:   Sat, 25 Nov 2023 15:39:09 +0000
From:   Benno Lossin <benno.lossin@...ton.me>
To:     Jarkko Sakkinen <jarkko@...nel.org>,
        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 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>`.

> 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.

>> 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

-- 
Cheers,
Benno

Powered by blists - more mailing lists