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

[Gary Guo <gary@garyguo.net>]

On Wed, 31 Dec 2025 23:05:21 -0500
Joel Fernandes <joelagnelf@nvidia.com> wrote:

> Hi John,
> 
> On 12/31/2025 9:52 PM, John Hubbard wrote:
> [..]
> >>> 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.  
> >>
> >> Chaining is not really only about error propagation. Builder pattern can be used
> >> for other cases too, like passing a setter chained expression to a function
> >> argument for better clarity, I was planning to do that for the sequencer for
> >> instance since there's a lot of parameters passed to it.  
> > 
> > Let's see if that has any use for this.
> > 
> > So far, though, in the code base that we have today, there is absolutely
> > zero benefit. The diffs here prove it.
> >   
> 
> From your patch diff, I see the lines of code increased. But that's not even the
> main issue I have with it (though IMO the chaining is more readable..).
> 
> >> But in this case, I am actually absolutely opposed against this, it makes the
> >> API hard to use because now how do you differentiate between an IO function call
> >> versus something that just mutates memory? Is set() an IO or write()?  
> > 
> > That's a completely separate, pre-existing issue with the API.  
> 
> Nope. With chaining we clearly know that the final operation is a write().
> 
> For instance, you cannot do:
>   reg.set_foo()
>      .write()
>      .set_bar()
> 
> That wont compile. You cannot intermingle write() with set_XX() because write()
> doesn't return anything that can be chained with. The builder pattern is typically:
>   obj.set_something()
>      .set_something()
>      .do_some_action()
> 
> The 'set' can also be 'with' from what I've seen, whatever. The point is the
> last thing is the action. IMO very readable and simple. I know that the write()
> will be what ends up doing the I/O. It is one entity that culminates in the write().
> 
> >   
> >>
> >>       reg.set_foo(x);   // no IO
> >>       reg.set_sec(y);
> >>       reg.write(bar);   // IO.
> >>
> >> So no thank you, I quite dislike it. :)
> >>
> >> Instead with chaining, we can just rely on the last part of the chain concluding
> >> in a write() with the intermediaries just mutating memory.  
> > 
> > Same as above, just a more happy-happy chaining interface, but the same
> > function calls must be made in the same order.  
> 
> No, you cannot place write() anywhere except at the end of the chain - the type
> system enforces this since write() returns ().

One thing that probably should be added though is `#[must_use]`
annotations on these set functions; this would ensure that if someone
writes

	reg.set_foo();

the compiler would complain that the return value is not used.

Best,
Gary