On Thu Mar 6, 2025 at 2:38 AM CET, Danilo Krummrich wrote:
> 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.
Aw well, can't have em all... Probably would be useful if you add a
comment saying that `expr` and `ty` can't be used.
>> > `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.
Sounds good.
---
Cheers,
Benno
