Re: [PATCH v2 4/4] rust: Move register and bitstruct macros out of Nova

Joel Fernandes
2025-09-08T17:06:21Z

On 9/7/2025 2:14 PM, Miguel Ojeda wrote:
> Hi Joel,
>
> I didn't check the macros, but a couple nits I noticed in this patch
> in particular given it moved it to `kernel`...
>
> On Wed, Sep 3, 2025 at 11:54 PM Joel Fernandes <joelagnelf@nvidia.com> wrote:
>>
>> +//! A library that provides support for defining bit fields in Rust
>
> What about just "Support for defining bit fields in ..." or similar?
>
>> +//! structures. Also used from things that need bitfields like register macro.

Ok, I changed it to:
"Also used from things that need bitfields like [`register!`] macro." for next
revision.

>
> The "register macro" part sounds like it should be formatted with
> Markdown plus an intra-doc link.
>
>> - ::kernel::build_assert!(
>> + build_assert!(
>
> Is this path unqualified for some reason? Does it mean the user needs
> to have imported the prelude?

Yes, for register macro importing prelude is required (I commented more below).

>
>> +pub use super::{bitstruct, register};
>
> Please justify in the commit message why we want them in the prelude,
> e.g. are they expected to be common? Does it simplify some code? etc.
>
The issue I ran into is, without adding it to prelude, the users of register!
macro will have to import both bitfield! and register! macros explictly, even
though they're only using register!. I tried to make it work without adding to
prelude, but couldn't:

use kernel::{bitfield, register};

Also not adding it to prelude, means register! macro has to invoke bitfield with
$crate prefixed ($crate::bitfield).

I think the prelude-way is clean, but let me know if there's any other trick I
can try.

I will also add this rationale to the commit message as you suggested.

Thanks!

- Joel