Message-Id: <DFZGOCV6VP2N.28M0CWIONBGMW@garyguo.net>
Date: Tue, 27 Jan 2026 15:02:51 +0000
From: "Gary Guo" <gary@...yguo.net>
To: "Joel Fernandes" <joelagnelf@...dia.com>, "Yury Norov"
 <ynorov@...dia.com>
Cc: "Alexandre Courbot" <acourbot@...dia.com>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "Danilo Krummrich" <dakr@...nel.org>,
 "Yury Norov" <yury.norov@...il.com>, "John Hubbard" <jhubbard@...dia.com>,
 "Alistair Popple" <apopple@...dia.com>, "Timur Tabi" <ttabi@...dia.com>,
 "Edwin Peer" <epeer@...dia.com>, "Eliot Courtney" <ecourtney@...dia.com>,
 "Daniel Almeida" <daniel.almeida@...labora.com>, "Dirk Behme"
 <dirk.behme@...bosch.com>, "Steven Price" <steven.price@....com>,
 <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/6] rust: add `bitfield!` macro

On Tue Jan 27, 2026 at 3:25 AM GMT, Joel Fernandes wrote:
> On Jan 26, 2026, at 9:55 PM, Yury Norov <ynorov@...dia.com> wrote:
>> On Mon, Jan 26, 2026 at 10:35:49PM +0900, Alexandre Courbot wrote:
>> > On Wed Jan 21, 2026 at 6:16 PM JST, Yury Norov wrote:
>> > > On Tue, Jan 20, 2026 at 03:17:56PM +0900, Alexandre Courbot wrote:
>> > > > Add a macro for defining bitfield structs with bounds-checked accessors.
>> > > >
>> > > > Each field is represented as a `Bounded` of the appropriate bit width,
>> > > > ensuring field values are never silently truncated.
>> > > >
>> > > > Fields can optionally be converted to/from custom types, either fallibly
>> > > > or infallibly.
>> > > >
>> > > > Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
>> > > > ---
>> > > > rust/kernel/bitfield.rs | 503 ++++++++++++++++++++++++++++++++++++++++++++++++
>> > > > rust/kernel/lib.rs      |   1 +
>> > > > 2 files changed, 504 insertions(+)
> [...]
>> > > > +/// // Setters can be chained. Bounded::new::<N>() does compile-time bounds checking.
>> > > > +/// let color = Rgb::default()
>> > > > +///     .set_red(Bounded::<u16, _>::new::<0x10>())
>> > > > +///     .set_green(Bounded::<u16, _>::new::<0x1f>())
>> > > > +///     .set_blue(Bounded::<u16, _>::new::<0x18>());
>> > >
>> > > Is there a way to just say:
>> > >
>> > >    let color = Rgb::default().
>> > >            .set_red(0x10)
>> > >            .set_green(0x1f)
>> > >            .set_blue(0x18)
>> > >
>> > > I think it should be the default style. Later in the patch you say:
>> > >
>> > >    Each field is internally represented as a [`Bounded`]
>> > >
>> > > So, let's keep implementation decoupled from an interface?
>> >
>> > That is unfortunately not feasible, but the syntax above should seldomly
>> > be used outside of examples.
>>
>> The above short syntax is definitely more desired over that wordy and
>> non-trivial version that exposes implementation internals.
>>
>> A regular user doesn't care of the exact mechanism that protects the
>> bitfields. He wants to just assign numbers to the fields, and let
>> your machinery to take care of the integrity.
>>
>> Can you please explain in details why that's not feasible, please
>> do it in commit message. If it's an implementation constraint,
>> please consider to re-implement.
>
> If the issue is the excessive turbofish syntax, how about a macro? For
> example:
>
>     let color = Rgb::default()
>         .set_red(bounded!(u16, 0x10))
>         .set_green(bounded!(u16, 0x1f))
>         .set_blue(bounded!(u16, 0x18));
>
> This hides the turbofish and Bounded internals while still providing
> compile-time bounds checking.

I think this could be the way forward, if we also get type inference working
properly.

    Rgb::default()
        .set_read(bounded!(0x10))
        .set_green(bounded!(0x1f))
        .set_blue(bounded!(0x18))

is roughly the limit that I find acceptable (`Bounded::<u16, _>::new::<0x10>()`
is something way too verbose so I find it unacceptable).

I still think if we can get 

    Rgb::default()
        .set_read(0x10)
        .set_green(0x1f)
        .set_blue(0x18)

to work with implicit `build_assert!` check it'll be ideal, although I
understand the concern about the fragility of `build_assert!()`, especially when
Clippy is used.

I am planning to at least improve the diagnostics when `build_assert!` is used
incorrectly and the build error actually occurs, so hopefully in the long run it
can once again become a tool that we can rely on, but in the meantime,
if all it needed is an extra `bounded!()` call, it doesn't bother me that much
versus the full turbofish.

Best,
Gary

>
> [...]
>> > > What Rgb::BLUE_SHIFT would mean in this case? Maybe Rgb::SHIFT(blue)?
>> >
>> > You wouldn't even have the luxury to yse `BLUE_SHIFT` here because where
>> > would be conflicting definitions and thus a build error.
> [...]
>> > > What color.set_blue() and color.into() would mean? Even if they work,
>> > > I think, to stay on safe side there should be a more conventional set
>> > > of accessors: color.get(into), color.set(set_blue, 0xff) and son on.
>> >
>> > This would just not build.
>>
>> I know it wouldn't. I am just trying to understand corner cases that may
>> (and would!) frustrate people for decades.
>>
>> I understand that this implementation works just great for the registers,
>> but my role is to make sure that it would work equally well for everyone.
>> Now, for example, Rust, contrary to C in Linux, actively uses camel case.
>> So, the blue vs Blue vs BLUE restriction is a very valid concern. The
>> same for reserved words like 'into'. As long as the API matures, the
>> number of such special words would only grow. The .shr() and .shl() that
>> you add in this series are the quite good examples.
>>
>> Let's make a step back and just list all limitations that come with this
>> implementation.
>
> Why is a build error not sufficient to alert the user to use better judgement
> for naming?
>
> This is no different than using reserved keywords in C. For example, this
> won't compile:
>
>     int if = 5;      // error: 'if' is a reserved keyword
>     int return = 3;  // error: 'return' is a reserved keyword
>
> The user simply learns not to use reserved words, and the compiler enforces
> this clearly. The same applies here.
>
>> Again, this all is relevant for a basic generic data structure. If we
>> consider it a supporting layer for the registers, everything is totally
>> fine. In that case, we should just give it a more specific name, and
>> probably place in an different directory, closer to IO APIs.
>
> The Bitfield macro is very much required for non-register use cases too.
>
> --
> Joel Fernandes

Powered by blists - more mailing lists