Re: [PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`

[Danilo Krummrich]

On Thu, Mar 06, 2025 at 12:24:21AM +0000, Benno Lossin wrote:
> On Thu Mar 6, 2025 at 12:57 AM CET, Danilo Krummrich wrote:
> > On Wed, Mar 05, 2025 at 11:36:54PM +0000, Benno Lossin wrote:
> >> On Wed Mar 5, 2025 at 11:38 PM CET, Danilo Krummrich wrote:
> >> > On Wed, Mar 05, 2025 at 10:30:31PM +0000, Benno Lossin wrote:
> >> >> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
> >> >> > +    /// Push an additional path component.
> >> >> > +    ///
> >> >> > +    /// After a new [`ModInfoBuilder`] instance has been created, [`ModInfoBuilder::prepare`] must
> >> >> > +    /// be called before adding path components.
> >> >> > +    pub const fn push(self, s: &str) -> Self {
> >> >> > +        if N != 0 && self.n == 0 {
> >> >> > +            crate::build_error!("Must call prepare() before push().");
> >> >>
> >> >> This will only prevent the first `prepare` call being missed, right?
> >> >
> >> > Correct, unfortunately there's no way to detect subsequent ones.
> >>
> >> Does it make sense to do that one in the constructor?
> >>
> >> (After looking at the example below) Ah maybe you can't do that, since
> >> then you would have two `prepare()` calls for the example below...?
> >
> > Exactly.
> >
> >> >> If you always have to call this before `push`, why not inline it there?
> >> >
> >> > You can push() multiple times to compose the firmware path string (which is the
> >> > whole purpose :).
> >>
> >> Ah I see, I only looked at the example you have in the next patch. All
> >> in all, I think this patch could use some better documentation, since I
> >> had to read a lot of the code to understand what everything is supposed
> >> to do...
> >
> > I can expand the example in module_firmware! to make things a bit more obvious.
> >
> > Otherwise, what information do you think is missing?
> 
> Abstractly: what `ModInfoBuilder` *does*, concretely:
> - why the generic constant `N` exists,

It doesn't really matter to the user, since the user never needs to supply it.
That happens in the module_firmware! macro.

I agree it not good to not mention anything about it at all, but I wouldn't want
to bother the user with all implemention details.

We can probably just mention that it's used internally and is supplied by
module_firmware!. (That module_firmware! does that by doing a dry run of the
builder itself, isn't necessary to know for the user I think.)

> - what `prepare()` does,

Same here, it's an implementation detail not relevant to the user. All the user
needs to know is that prepare() acts as a separator to be able to supply the
next firmware path.

> - what happens with the `module_name` parameter of `new`

Should probably just mention it's supplied by module_firmware! and used
internally.

> - answer the question "I want that the builder outputs the string `???`
>   can it do that? If yes, how do I do it?"

All it does is concatenating multiple &str in const context, which I thought is
clear since there are only push() and prepare() as public methods.

May it be that your request is more about can we add more hints on the
implementation details rather than user focused documentation?