Re: [PATCH 2/3] rust: macros: allow generic parameter default values in `#[pin_data]`

[Greg KH <gregkh@linuxfoundation.org>]

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