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 <[email protected]> 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
