Messages in this thread | | | Date | Sat, 02 Dec 2023 19:33:45 +0200 | From | "Jarkko Sakkinen" <> | 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
| |