On Fri, 19 Sep 2025 01:26:28 +0200
"Danilo Krummrich" <[email protected]> wrote:
> On Thu Sep 18, 2025 at 8:13 PM CEST, Joel Fernandes wrote:
> > On Thu, Sep 18, 2025 at 03:02:11PM +0000, Alice Ryhl wrote:
> >> Using build_assert! to assert that offsets are in bounds is really
> >> fragile and likely to result in spurious and hard-to-debug build
> >> failures. Therefore, build_assert! should be avoided for this case.
> >> Thus, update the code to perform the check in const evaluation instead.
> >
> > I really don't think this patch is a good idea (and nobody I spoke to thinks
> > so). Not only does it mess up the user's caller syntax completely, it is
> > also
>
> I appreacite you raising the concern, but I rather have other people speak up
> themselves.
Joel asked me for opinion on this syntax during Kangrejos and I said
that I hated it.
>
> > super confusing to pass both a generic and a function argument separately.
>
> Why? We assert that the offset has to be const, whereas the value does not
> have this requirement, so this makes perfect sense to me.
In some peripherals there are also cases where there are a window of
registers and you want to use a computed value to access them.
If I have a window of registers where the first address is `FOO` and
the last is `FOO_END`, then I might reasonable want to do access
registers in a loop.
>
> (I agree that it can look unfamiliar at the first glance though.)
>
> > Sorry if I knew this would be the syntax, I would have objected even when we
> > spoke :)
> >
> > I think the best fix (from any I've seen so far), is to move the bindings
> > calls of offending code into a closure and call the closure directly, as I
> > posted in the other thread. I also passed the closure idea by Gary and he
> > confirmed the compiler should behave correctly (I will check the code gen
> > with/without later). Gary also provided a brilliant suggestion that we can
> > call the closure directly instead of assigning it to a variable first. That
> > fix is also smaller, and does not screw up the users. APIs should fix issues
> > within them instead of relying on user to work around them.
>
> This is not a workaround, this is an idiomatic solution (which I probably
> should
> have been doing already when I introduced the I/O code).
I don't think this can be said to be idiomatic. Syntax churn is a real
issue, and quite a big one.
Turbofish is cumbersome to write with just magic numbers, and the
fact `{}` is needed to pass in constant expressions made this much
worse. If the drivers try hard to avoid magic numbers, you would
effective require all code to be `::<{ ... }>()` and this is ugly.
In the design of generic atomics this is taken into consideration and
is why the `load`/`store` functions don't take `Ordering` type
parameter itself, but additionally takes a value of it and just throw it
away. This is so that you can still write `.load(Relaxed)` rather
than needing to write `.load::<Relaxed>()`.
If we have argument position const generics today, I'd say let's make
the switch. However with the tools that we have I don't think it's a
clear cut. I would even personally prefer a BUG than to having to do
turbofish every single time.
Best,
Gary
>
> We do exactly the same thing for DmaMask::new() [1] and we agreed on doing the
> same thing for Alignment as well [2].
>
> [1] https://rust.docs.kernel.org/kernel/dma/struct.DmaMask.html#method.new
> [2]
> https://lore.kernel.org/rust-for-linux/[email protected]/
