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.
This patch adds the `decl_generics` which can only be used on type
defintions, since they contain the default values for generic
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.
Signed-off-by: Benno Lossin <[email protected]>
---
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
--
2.40.1
This patch adds compatibilty for generic parameters defaults by using
the newly introduced `decl_generics` instead of the `impl_generics`.
Signed-off-by: Benno Lossin <[email protected]>
---
rust/kernel/init/macros.rs | 19 ++++++++++++++++++-
rust/macros/pin_data.rs | 3 ++-
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
index cb6e61b6c50b..624e9108e3b4 100644
--- a/rust/kernel/init/macros.rs
+++ b/rust/kernel/init/macros.rs
@@ -538,6 +538,7 @@ macro_rules! __pin_data {
),
@impl_generics($($impl_generics:tt)*),
@ty_generics($($ty_generics:tt)*),
+ @decl_generics($($decl_generics:tt)*),
@body({ $($fields:tt)* }),
) => {
// We now use token munching to iterate through all of the fields. While doing this we
@@ -560,6 +561,9 @@ macro_rules! __pin_data {
@impl_generics($($impl_generics)*),
// The 'ty generics', the generics that will need to be specified on the impl blocks.
@ty_generics($($ty_generics)*),
+ // The 'decl generics', the generics that need to be specified on the struct
+ // definition.
+ @decl_generics($($decl_generics)*),
// The where clause of any impl block and the declaration.
@where($($($whr)*)?),
// The remaining fields tokens that need to be processed.
@@ -585,6 +589,7 @@ macro_rules! __pin_data {
@name($name:ident),
@impl_generics($($impl_generics:tt)*),
@ty_generics($($ty_generics:tt)*),
+ @decl_generics($($decl_generics:tt)*),
@where($($whr:tt)*),
// We found a PhantomPinned field, this should generally be pinned!
@fields_munch($field:ident : $($($(::)?core::)?marker::)?PhantomPinned, $($rest:tt)*),
@@ -607,6 +612,7 @@ macro_rules! __pin_data {
@name($name),
@impl_generics($($impl_generics)*),
@ty_generics($($ty_generics)*),
+ @decl_generics($($decl_generics)*),
@where($($whr)*),
@fields_munch($($rest)*),
@pinned($($pinned)* $($accum)* $field: ::core::marker::PhantomPinned,),
@@ -623,6 +629,7 @@ macro_rules! __pin_data {
@name($name:ident),
@impl_generics($($impl_generics:tt)*),
@ty_generics($($ty_generics:tt)*),
+ @decl_generics($($decl_generics:tt)*),
@where($($whr:tt)*),
// We reached the field declaration.
@fields_munch($field:ident : $type:ty, $($rest:tt)*),
@@ -640,6 +647,7 @@ macro_rules! __pin_data {
@name($name),
@impl_generics($($impl_generics)*),
@ty_generics($($ty_generics)*),
+ @decl_generics($($decl_generics)*),
@where($($whr)*),
@fields_munch($($rest)*),
@pinned($($pinned)* $($accum)* $field: $type,),
@@ -656,6 +664,7 @@ macro_rules! __pin_data {
@name($name:ident),
@impl_generics($($impl_generics:tt)*),
@ty_generics($($ty_generics:tt)*),
+ @decl_generics($($decl_generics:tt)*),
@where($($whr:tt)*),
// We reached the field declaration.
@fields_munch($field:ident : $type:ty, $($rest:tt)*),
@@ -673,6 +682,7 @@ macro_rules! __pin_data {
@name($name),
@impl_generics($($impl_generics)*),
@ty_generics($($ty_generics)*),
+ @decl_generics($($decl_generics)*),
@where($($whr)*),
@fields_munch($($rest)*),
@pinned($($pinned)*),
@@ -689,6 +699,7 @@ macro_rules! __pin_data {
@name($name:ident),
@impl_generics($($impl_generics:tt)*),
@ty_generics($($ty_generics:tt)*),
+ @decl_generics($($decl_generics:tt)*),
@where($($whr:tt)*),
// We found the `#[pin]` attr.
@fields_munch(#[pin] $($rest:tt)*),
@@ -705,6 +716,7 @@ macro_rules! __pin_data {
@name($name),
@impl_generics($($impl_generics)*),
@ty_generics($($ty_generics)*),
+ @decl_generics($($decl_generics)*),
@where($($whr)*),
@fields_munch($($rest)*),
// We do not include `#[pin]` in the list of attributes, since it is not actually an
@@ -724,6 +736,7 @@ macro_rules! __pin_data {
@name($name:ident),
@impl_generics($($impl_generics:tt)*),
@ty_generics($($ty_generics:tt)*),
+ @decl_generics($($decl_generics:tt)*),
@where($($whr:tt)*),
// We reached the field declaration with visibility, for simplicity we only munch the
// visibility and put it into `$accum`.
@@ -741,6 +754,7 @@ macro_rules! __pin_data {
@name($name),
@impl_generics($($impl_generics)*),
@ty_generics($($ty_generics)*),
+ @decl_generics($($decl_generics)*),
@where($($whr)*),
@fields_munch($field $($rest)*),
@pinned($($pinned)*),
@@ -757,6 +771,7 @@ macro_rules! __pin_data {
@name($name:ident),
@impl_generics($($impl_generics:tt)*),
@ty_generics($($ty_generics:tt)*),
+ @decl_generics($($decl_generics:tt)*),
@where($($whr:tt)*),
// Some other attribute, just put it into `$accum`.
@fields_munch(#[$($attr:tt)*] $($rest:tt)*),
@@ -773,6 +788,7 @@ macro_rules! __pin_data {
@name($name),
@impl_generics($($impl_generics)*),
@ty_generics($($ty_generics)*),
+ @decl_generics($($decl_generics)*),
@where($($whr)*),
@fields_munch($($rest)*),
@pinned($($pinned)*),
@@ -789,6 +805,7 @@ macro_rules! __pin_data {
@name($name:ident),
@impl_generics($($impl_generics:tt)*),
@ty_generics($($ty_generics:tt)*),
+ @decl_generics($($decl_generics:tt)*),
@where($($whr:tt)*),
// We reached the end of the fields, plus an optional additional comma, since we added one
// before and the user is also allowed to put a trailing comma.
@@ -802,7 +819,7 @@ macro_rules! __pin_data {
) => {
// Declare the struct with all fields in the correct order.
$($struct_attrs)*
- $vis struct $name <$($impl_generics)*>
+ $vis struct $name <$($decl_generics)*>
where $($whr)*
{
$($fields)*
diff --git a/rust/macros/pin_data.rs b/rust/macros/pin_data.rs
index 022e68e9720d..1d4a3547c684 100644
--- a/rust/macros/pin_data.rs
+++ b/rust/macros/pin_data.rs
@@ -10,7 +10,7 @@ pub(crate) fn pin_data(args: TokenStream, input: TokenStream) -> TokenStream {
let (
Generics {
impl_generics,
- decl_generics: _,
+ decl_generics,
ty_generics,
},
rest,
@@ -77,6 +77,7 @@ pub(crate) fn pin_data(args: TokenStream, input: TokenStream) -> TokenStream {
@sig(#(#rest)*),
@impl_generics(#(#impl_generics)*),
@ty_generics(#(#ty_generics)*),
+ @decl_generics(#(#decl_generics)*),
@body(#last),
});
quoted.extend(errs);
--
2.40.1
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 <[email protected]>
> ---
> 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
On Sat, Nov 25, 2023 at 12:51:09PM +0000, Benno Lossin wrote:
> This patch adds compatibilty for generic parameters defaults by using
> the newly introduced `decl_generics` instead of the `impl_generics`.
This says _what_ is happening here, but not _why_ this is needed at all.
Try taking a look a the kernel documentation for how to write a good
changelog text to make this much better. It's often times the most
difficult portion of making a kernel patch, the code is easy, writing a
summary of why everyone else should agree that this code is acceptable
is hard.
good luck!
greg k-h
On 25.11.23 15:26, Greg KH wrote:
> On Sat, Nov 25, 2023 at 12:51:09PM +0000, Benno Lossin wrote:
>> This patch adds compatibilty for generic parameters defaults by using
>> the newly introduced `decl_generics` instead of the `impl_generics`.
>
> This says _what_ is happening here, but not _why_ this is needed at all.
>
> Try taking a look a the kernel documentation for how to write a good
> changelog text to make this much better. It's often times the most
> difficult portion of making a kernel patch, the code is easy, writing a
> summary of why everyone else should agree that this code is acceptable
> is hard.
The reason is hidden in the third patch. Without this, the `#[pin_data]
macro would not allow specifying const generic parameter default values
and instead emit a compile error. I will add this to v2.
--
Cheers,
Benno
On Sat, Nov 25, 2023 at 03:02:00PM +0000, Benno Lossin wrote:
> On 25.11.23 15:26, Greg KH wrote:
> > On Sat, Nov 25, 2023 at 12:51:09PM +0000, Benno Lossin wrote:
> >> This patch adds compatibilty for generic parameters defaults by using
> >> the newly introduced `decl_generics` instead of the `impl_generics`.
> >
> > This says _what_ is happening here, but not _why_ this is needed at all.
> >
> > Try taking a look a the kernel documentation for how to write a good
> > changelog text to make this much better. It's often times the most
> > difficult portion of making a kernel patch, the code is easy, writing a
> > summary of why everyone else should agree that this code is acceptable
> > is hard.
>
> The reason is hidden in the third patch.
Please do not hide things, patches need to be stand-alone and
understandable that way, otherwise they will be rejected as no one can
understand why they would be needed.
> Without this, the `#[pin_data]
> macro would not allow specifying const generic parameter default values
> and instead emit a compile error.
That's nice, but it still doesn't tell me _why_ this is needed. Why
would I want any generic paramter default values at all? Who needs any
of this? What will it be used for? What does it actually do?
thanks,
greg k-h
On 25.11.23 16:10, Greg KH wrote:
> On Sat, Nov 25, 2023 at 03:02:00PM +0000, Benno Lossin wrote:
>> On 25.11.23 15:26, Greg KH wrote:
>>> On Sat, Nov 25, 2023 at 12:51:09PM +0000, Benno Lossin wrote:
>>>> This patch adds compatibilty for generic parameters defaults by using
>>>> the newly introduced `decl_generics` instead of the `impl_generics`.
>>>
>>> This says _what_ is happening here, but not _why_ this is needed at all.
>>>
>>> Try taking a look a the kernel documentation for how to write a good
>>> changelog text to make this much better. It's often times the most
>>> difficult portion of making a kernel patch, the code is easy, writing a
>>> summary of why everyone else should agree that this code is acceptable
>>> is hard.
>>
>> The reason is hidden in the third patch.
>
> Please do not hide things, patches need to be stand-alone and
> understandable that way, otherwise they will be rejected as no one can
> understand why they would be needed.
This was not my intention, I just realized this due to your question. I
wanted to point you to the third patch (which for some reason sadly does
not have the correct In-Reply-To header). Since that might help you
understand some of the context.
>> Without this, the `#[pin_data]
>> macro would not allow specifying const generic parameter default values
>> and instead emit a compile error.
>
> That's nice, but it still doesn't tell me _why_ this is needed. Why
> would I want any generic paramter default values at all? Who needs any
> of this? What will it be used for? What does it actually do?
`#[pin_data]` is a proc-macro that one can put on any struct to make the
pin-init API available for use with that struct. Since e.g. mutexes are
initialized using the pin-init API, you have to do this for anything
that contains a mutex.
This macro should be compatible with any struct definition even with
ones that have const generic parameter defaults. This was an oversight
in the original design, as it does not support that, since the proc
macro parsing cannot handle the `=` character.
The short answer for why one would want to have const generic parameter
defaults is that the language supports it. And since there is nothing
that prevents `#[pin_data]` to be implemented for such structs, we
should it do it.
Rust generally aims to make all features compatible
with each other and we would like to do the same for our
libraries/customized features.
The longer answer is a concrete example of a usecase for const generic
parameter defaults: the `Work<T, ID>` struct of the workqueue bindings.
The `ID` parameter is used to identify multiple instances of `Work`
within the same struct. But if you only intend to have a single `Work`
struct embedded in your struct, then there is no need to distinguish it
from something else (after all there is only one) and therefore we want
people to just write `Work<T>`. This is where the author of
`Work<T, ID>` can write:
struct Work<T, const ID: usize = 0> {
// ...
}
But the `= 0` syntax is currently not supported by `#[pin_data]`.
--
Cheers,
Benno
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
On Sat, Nov 25, 2023 at 03:26:02PM +0000, Benno Lossin wrote:
> On 25.11.23 16:10, Greg KH wrote:
> > On Sat, Nov 25, 2023 at 03:02:00PM +0000, Benno Lossin wrote:
> >> On 25.11.23 15:26, Greg KH wrote:
> >>> On Sat, Nov 25, 2023 at 12:51:09PM +0000, Benno Lossin wrote:
> >>>> This patch adds compatibilty for generic parameters defaults by using
> >>>> the newly introduced `decl_generics` instead of the `impl_generics`.
> >>>
> >>> This says _what_ is happening here, but not _why_ this is needed at all.
> >>>
> >>> Try taking a look a the kernel documentation for how to write a good
> >>> changelog text to make this much better. It's often times the most
> >>> difficult portion of making a kernel patch, the code is easy, writing a
> >>> summary of why everyone else should agree that this code is acceptable
> >>> is hard.
> >>
> >> The reason is hidden in the third patch.
> >
> > Please do not hide things, patches need to be stand-alone and
> > understandable that way, otherwise they will be rejected as no one can
> > understand why they would be needed.
>
> This was not my intention, I just realized this due to your question. I
> wanted to point you to the third patch (which for some reason sadly does
> not have the correct In-Reply-To header). Since that might help you
> understand some of the context.
Again, changes need to be understood on their own, so provide some hints
as to _why_ this is needed.
> >> Without this, the `#[pin_data]
> >> macro would not allow specifying const generic parameter default values
> >> and instead emit a compile error.
> >
> > That's nice, but it still doesn't tell me _why_ this is needed. Why
> > would I want any generic paramter default values at all? Who needs any
> > of this? What will it be used for? What does it actually do?
>
> `#[pin_data]` is a proc-macro that one can put on any struct to make the
> pin-init API available for use with that struct. Since e.g. mutexes are
> initialized using the pin-init API, you have to do this for anything
> that contains a mutex.
> This macro should be compatible with any struct definition even with
> ones that have const generic parameter defaults. This was an oversight
> in the original design, as it does not support that, since the proc
> macro parsing cannot handle the `=` character.
>
> The short answer for why one would want to have const generic parameter
> defaults is that the language supports it.
Wait, no, that's not what we do in the kernel. We only add support for
things that we actually need and use.
If you have no use for this, but it's here just "because we might want
it someday", then we can't take it for obvious reasons.
So provide a user of the feature, and then we can actually understand if
it is worth adding, or perhaps, it's not needed at all as other things
can be done.
> And since there is nothing
> that prevents `#[pin_data]` to be implemented for such structs, we
> should it do it.
> Rust generally aims to make all features compatible
> with each other and we would like to do the same for our
> libraries/customized features.
The kernel doesn't have a "library", that's not how we work, it's
self-contained and does not export anything nor work with external
libraries outside of its source tree.
> The longer answer is a concrete example of a usecase for const generic
> parameter defaults: the `Work<T, ID>` struct of the workqueue bindings.
> The `ID` parameter is used to identify multiple instances of `Work`
> within the same struct.
Why not just declare them as different names?
And multiple workqueues in a single structure are ripe for problems, are
you sure you need that?
> But if you only intend to have a single `Work`
> struct embedded in your struct, then there is no need to distinguish it
> from something else (after all there is only one) and therefore we want
> people to just write `Work<T>`. This is where the author of
> `Work<T, ID>` can write:
>
> struct Work<T, const ID: usize = 0> {
> // ...
> }
>
> But the `= 0` syntax is currently not supported by `#[pin_data]`.
Why not just force a name for either way it is declared? Wait, "id"?
What is that for and what will require and define that?
thanks,
greg k-h
On 11/25/23 17:03, Greg KH wrote:
>>>> Without this, the `#[pin_data]
>>>> macro would not allow specifying const generic parameter default values
>>>> and instead emit a compile error.
>>>
>>> That's nice, but it still doesn't tell me _why_ this is needed. Why
>>> would I want any generic paramter default values at all? Who needs any
>>> of this? What will it be used for? What does it actually do?
>>
>> `#[pin_data]` is a proc-macro that one can put on any struct to make the
>> pin-init API available for use with that struct. Since e.g. mutexes are
>> initialized using the pin-init API, you have to do this for anything
>> that contains a mutex.
>> This macro should be compatible with any struct definition even with
>> ones that have const generic parameter defaults. This was an oversight
>> in the original design, as it does not support that, since the proc
>> macro parsing cannot handle the `=` character.
>>
>> The short answer for why one would want to have const generic parameter
>> defaults is that the language supports it.
>
> Wait, no, that's not what we do in the kernel. We only add support for
> things that we actually need and use.
>
> If you have no use for this, but it's here just "because we might want
> it someday", then we can't take it for obvious reasons.
>
> So provide a user of the feature, and then we can actually understand if
> it is worth adding, or perhaps, it's not needed at all as other things
> can be done.
Here's how I see the proposed change: "The workqueue abstractions has to
use a backdoor to implement something because the safe and more
convenient API doesn't support it. Improve the safe API so that the
workqueue does not need the backdoor, then update the workqueue to not
use the backdoor."
>> And since there is nothing
>> that prevents `#[pin_data]` to be implemented for such structs, we
>> should it do it.
>> Rust generally aims to make all features compatible
>> with each other and we would like to do the same for our
>> libraries/customized features.
>
> The kernel doesn't have a "library", that's not how we work, it's
> self-contained and does not export anything nor work with external
> libraries outside of its source tree.
I guess this is a question of terminology. What do you call the kernel's
xarray if not a "library" for use by the rest of the kernel?
>> The longer answer is a concrete example of a usecase for const generic
>> parameter defaults: the `Work<T, ID>` struct of the workqueue bindings.
>> The `ID` parameter is used to identify multiple instances of `Work`
>> within the same struct.
>
> Why not just declare them as different names?
I would have preferred to use a textual name rather than an id, but
const generics currently only supports integers.
> And multiple workqueues in a single structure are ripe for problems, are
> you sure you need that?
Originally I had this in Binder for deferring both "flush" and "close".
However, I changed that and now I use a bitfield to keep track of
whether we need a flush or close. (So that if both operations are
scheduled, I can guarantee that I run the flush operation first.)
We could remove the ID from the workqueue abstractions now that I no
longer need it, but it would not really simplify that much in the
workqueue abstraction. Its complexity comes from having to embed the
work_struct inside a user-controlled struct, and once you have to
support that, supporting exactly one or any number of work_struct fields
is about the same difficulty.
The linked list abstraction (which I have not yet sent to the mailing
list) has the same feature, and there, Rust Binder actually *does* need
a single struct to have multiple list_head fields in some places, so at
least the current state means that these APIs are more consistent with
each other.
>> But if you only intend to have a single `Work`
>> struct embedded in your struct, then there is no need to distinguish it
>> from something else (after all there is only one) and therefore we want
>> people to just write `Work<T>`. This is where the author of
>> `Work<T, ID>` can write:
>>
>> struct Work<T, const ID: usize = 0> {
>> // ...
>> }
>>
>> But the `= 0` syntax is currently not supported by `#[pin_data]`.
>
> Why not just force a name for either way it is declared? Wait, "id"?
> What is that for and what will require and define that?
Each work_struct field specifies an id as part of its type, and when you
call `enqueue`, you use the same id to specify which work_struct to
enqueue to the workqueue. The ids are purely a compile-time thing, and
do not exist at runtime. If you give it an id for which there is no
corresponding field, it will fail to compile. If you use the same id for
two fields in the same struct, it will fail to compile. The id has to be
a compile-time constant.
Furthermore, since the workqueue uses a default parameter, you only have
to specify the id if you have multiple work_struct fields.
Alice
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
On 12/2/23 18:33, Jarkko Sakkinen wrote:
> On Sat Nov 25, 2023 at 5:39 PM EET, Benno Lossin wrote:
>> On 25.11.23 14:39, Jarkko Sakkinen wrote:
>>> On Sat, 2023-11-25 at 12:50 +0000, Benno Lossin wrote:
>>>> 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.
No worries!
> 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.
I agree. One thing that we are already doing is encouraging
documentation, see [1]. We also use `-Wmissing_docs` which makes the
compiler emit a warning when you have a public item (a function,
constant, type etc.) that has no documentation.
Rust documentation is placed directly next to the source code. This
helps keep it up to date and makes it easier to find documentation.
[1]: https://docs.kernel.org/rust/coding-guidelines.html#code-documentation
Additionally we can make commit messages more elaborate. Though we
probably need help with choosing the topics and depth of the
explanations. Since when you are already familiar with something, it
is often difficult to know what is hard to understand. This also holds
for documentation.
That being said, I think that we should not document common Rust
patterns and properties. I think you understand that we do not want
to write code like this over-the-top example:
void init_foo(struct foo *foo)
{
// `foo->value` is short for `(*foo).value`
foo->value = 42;
}
I am of course open to help anyone understand the Rust code that
we upstream, by giving pointers to the relevant Rust resources (e.g.
the Rust book or std library documentation) or explaining our custom
code. We might even find something that should be documented better
and then I will be happy to do so.
--
Cheers,
Benno