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

[Danilo Krummrich]

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?

> 
> It might also make sense to make this more generic, since one probably
> also needs this in other places? Or do you think this will only be
> required for modinfo?

Currently, I don't think there's any more need for a generic const string
builder. For now, I think we're good. Let's factor it out, once we have actual
need for that.