Hello, Danilo, > On Sep 19, 2025, at 1:26 AM, Danilo Krummrich <d...@kernel.org> 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.
I did not mean to speak for others, sorry it came across like that (and that is certainly not what I normally do). But I discussed the patch in person since we are at a conference and discussing it in person, and I did not get a lot of consensus on this. That is what I was trying to say. If it was a brilliant or great idea, I would have hoped for at least one person to tell me that this is exactly how we should do 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. > > (I agree that it can look unfamiliar at the first glance though.) Yes the familiarity is the issue. I still do not feel using a generic for this looks ok to me and I think we can fix it differently, I will try to come up with an alternative fix unless we have already decided to use generics for this. > >> 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). > > 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/20250908-num-v5-1-c0f2f681e...@nvidia.com/ Ah ok. Since there is precedent, I am ok with it, especially since you feel so strongly about it and since you are the rust IO code maintainer. But passing one parameter as a constant generic and another parameter as a function parameter, just seems weird to me even if there is precedent. And one can argue that the value is also a constant right? It is confusing and makes callers unreadable IMHO. Cheers, - Joel
