Re: [PATCH] gpu: nova-core: bitfield: use &mut self setters instead of builder pattern

[John Hubbard]

On 12/31/25 4:15 PM, Joel Fernandes wrote:
>> On Dec 31, 2025, at 5:47 PM, John Hubbard <jhubbard@xxxxxxxxxx> wrote:
>>
>> On 12/31/25 2:33 PM, Timur Tabi wrote:
>>>> On Wed, 2025-12-31 at 13:47 -0800, John Hubbard wrote:
>>>> The builder-pattern setters (self -> Self) enabled method chaining like:
>>>>
>>>>     reg.set_foo(x).set_sec(y).write(bar);
>>>>
>>>> This made separate operations appear as a single expression, obscuring
>>>> that each setter is a distinct mutation. 
>>>
>>> So you're concerned about the fact that the compiler is not merging the set_foo(x) and the
>>> set_sec(y) into a single read-modify-write?
>>
>> No, I don't care about that aspect.
>>
>>>
>>>> These setters are infallible,
>>>> so the chaining provides no error-propagation benefit—it just obscures
>>>> what are simple, independent assignments.
>>>>
>>>> Change the bitfield!() macro to generate `&mut self` setters, so each
>>>> operation is a distinct statement:
>>>>
>>>>     reg.set_foo(x);
>>>>     reg.set_sec(y);
>>>>     reg.write(bar);
>>>
>>> Are you sure about this?  It just seems like you're throwing out a neat little feature of Rust and
>>> replacing it with something that's very C-like.  This breaks compatible with all users of the regs
>>> macros.  Seems really disruptive for what seems to me like a cosmetic change.
>>>
>>
>> It's only a neat feature if it *does* something. In this case, it *looks*
>> like a neat Rust feature, but under the covers it really is just handing
>> around copies unnecessarily, when really, it *is* doing the C-like thing
>> in the end.
>>
>> I object to the fake Rust-ness that's being done here. It's like putting
>> hubcabs on a car.
> 
> But IMO there is only one operation here, the IO write. The setter is just mutations. Builder pattern chaining is idiomatic to Rust.
> 
> I would not call it fake Rustness since it is Rustness in the Rust language. Afair, similar arguments were made before and conclusion was, well, this is Rust.

There is nothing about doing sequential .set_foo() calls that goes against
Rust idioms.

But this really is fake chaining, because there are no Results involved.
It's not buying us anything except a bit of indirection and cognitive
load for the reader.

> 
> Regarding the copies, I am intrigued - have you verified that the code generation actually results in copies? I would be surprised if the compiler did not optimize.


No no, I just mean conceptually using Copy instead of a mutable Self.

thanks,
-- 
John Hubbard