On Thu, Mar 06, 2025 at 01:27:19AM +0000, Benno Lossin wrote:
> On Thu Mar 6, 2025 at 2:04 AM CET, Danilo Krummrich wrote:
> > On Thu, Mar 06, 2025 at 12:31:14AM +0000, Benno Lossin wrote:
> >> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
> >>
> >> > +#[macro_export]
> >> > +macro_rules! module_firmware {
> >> > + ($($builder:tt)*) => {
> >>
> >> This should probably be `$builder:expr` instead.
> >
> > That doesn't work, the compiler then complains, since it's not an expression:
> >
> > 193 | static __MODULE_FIRMWARE: [u8; $builder::create(__module_name()).build_length()] =
> > | ^^ expected one of `.`, `?`, `]`, or an operator
>
> Does `<$builder>::create` work (with the `expr` fragment)?
No, the compiler then explicitly complains that it expects a type.
>
> > `ty` doesn't work either, since then the compiler expects the caller to add the
> > const generic, which we want the macro to figure out instead.
> >
> >>
> >> > +
> >> > + #[cfg(not(MODULE))]
> >> > + const fn __module_name() -> &'static kernel::str::CStr {
> >> > + <LocalModule as kernel::ModuleMetadata>::NAME
> >>
> >> Please either use `::kernel::` or `$crate::` instead of `kernel::`.
> >
> > Good catch, thanks.
> >
> >>
> >> Hmm, I am not 100% comfortable with the `LocalModule` way of accessing
> >> the current module for some reason, no idea if there is a rational
> >> argument behind that, but it just doesn't sit right with me.
> >>
> >> Essentially you're doing this for convenience, right? So you don't want
> >> to have to repeat the name of the module type every time?
> >
> > No, it's really that I can't know the type name here, please see the previous
> > patch commit message that introduces `LocalModule` for explanation.
>
> Gotcha.
>
> >> > + }
> >> > +
> >> > + #[cfg(MODULE)]
> >> > + const fn __module_name() -> &'static kernel::str::CStr {
> >> > + kernel::c_str!("")
> >>
> >> Ditto.
> >>
> >> > + }
> >>
> >> Are these two functions used outside of the `static` below? If no, then
> >> you can just move them into the static? You can also probably use a
> >> `const` instead of a function, that way you only have 4 lines instead
> >> of 8.
> >
> > Is this what you're proposing?
> >
> > #[macro_export]
> > macro_rules! module_firmware {
> > ($($builder:tt)*) => {
> > const __MODULE_FIRMWARE_PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
> > $crate::c_str!("")
> > } else {
> > <LocalModule as $crate::ModuleMetadata>::NAME
> > };
> >
> > #[link_section = ".modinfo"]
> > #[used]
> > static __MODULE_FIRMWARE: [u8; $($builder)*::create(__MODULE_FIRMWARE_PREFIX)
> > .build_length()] = $($builder)*::create(__MODULE_FIRMWARE_PREFIX).build();
>
> I meant to also move the `const` into the expression, but I guess that
> leads to duplication:
>
> #[link_section = ".modinfo"]
> #[used]
> static __MODULE_FIRMWARE: [u8; {
> const PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
> $crate::c_str!("")
> } else {
> <LocalModule as $crate::ModuleMetadata>::NAME
> };
> <$builder>::create(PREFIX).build_length()
> }] = {
> const PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
> $crate::c_str!("")
> } else {
> <LocalModule as $crate::ModuleMetadata>::NAME
> };
> <$builder>::create(PREFIX)
> };
>
> But then the advantage is that only the `__MODULE_FIRMWARE` static will
> be in-scope.
>
> Do you think that its useful to have the static be accessible? I.e. do
> users need to access it (I would think they don't)? If they don't, then
> we could put all of those things into a `const _: () = { /* ... */ };`.
> But then people can invoke `module_firmware!` multiple times in the same
> module, is that a problem?
Didn't know that's possible (const _; () = { ... };). That's pretty nice, I will
go with my above proposal wrapped into the anonymous const. Thanks.
